From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 71555 invoked by alias); 3 Apr 2018 15:12:25 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 71543 invoked by uid 89); 3 Apr 2018 15:12:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.4 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: mailbackend.panix.com X-Gm-Message-State: ALQs6tByY94f7JUhMsVTQtjYJPgDKzpeK6gJzouQtDKVWsvhC0Eg4rpr YN5MvWTI6IMLRCUrUb+fGFFbHFU6n/hO6ECo/8U= X-Google-Smtp-Source: AIpwx48ClIH7VdDZmaG82d4S+28BfEGOxW5TtQZa7FV7JmptBzbysD7LoR+dMpj1jlKSWO7jO1bxgeFpi8NnNmeS2iU= X-Received: by 2002:aca:bf57:: with SMTP id p84-v6mr8189383oif.64.1522768338218; Tue, 03 Apr 2018 08:12:18 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <4a423a86-4de3-1830-4c06-d034282fbaed@linaro.org> References: <1518439345-6013-1-git-send-email-adhemerval.zanella@linaro.org> <1518439345-6013-2-git-send-email-adhemerval.zanella@linaro.org> <43cc9193-1cea-6ae3-388e-88bb3406ac9c@linaro.org> <4a423a86-4de3-1830-4c06-d034282fbaed@linaro.org> From: Zack Weinberg Date: Tue, 03 Apr 2018 15:12:00 -0000 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v4 2/4] Filter out NPTL internal signals (BZ #22391) To: Adhemerval Zanella Cc: GNU C Library Content-Type: text/plain; charset="UTF-8" X-SW-Source: 2018-04/txt/msg00059.txt.bz2 LGTM except I think sysdeps/generic/internal-signals.h should define all of the functions that sysdeps/u/s/l/internal-signals.h does. On Tue, Apr 3, 2018 at 10:24 AM, Adhemerval Zanella wrote: > If no one opposes, I will commit this shortly. > > On 19/02/2018 14:50, Adhemerval Zanella wrote: >> Ping. >> >> On 12/02/2018 10:42, Adhemerval Zanella wrote: >>> This patch filters out the internal NPTL signals (SIGCANCEL/SIGTIMER and >>> SIGSETXID) from signal functions. GLIBC on Linux requires both signals to >>> proper implement pthread cancellation, posix timers, and set*id posix >>> thread synchronization. >>> >>> And not filtering out the internal signal is troublesome: >>> >>> - A conformant program on a architecture that does not filter out the >>> signals might inadvertently disable pthread asynchronous cancellation, >>> set*id synchronization or posix timers. >>> >>> - It might also to security issues if SIGSETXID is masked and set*id >>> functions are called (some threads might have effective user or group >>> id different from the rest). >>> >>> The changes are basically: >>> >>> - Change __is_internal_signal to bool and used on all signal function >>> that has a signal number as input. Also for signal function which accepts >>> signals sets (sigset_t) it assumes that canonical function were used to >>> add/remove signals which lead to some input simplification. >>> >>> - Fix tst-sigset.c to avoid check for SIGCANCEL/SIGTIMER and SIGSETXID. >>> It is rewritten to check each signal indidually and to check realtime >>> signals using canonical macros. >>> >>> - Add generic __clear_internal_signals and __is_internal_signal >>> version since both symbols are used on generic implementations. >>> >>> - Remove superflous sysdeps/nptl/sigfillset.c. >>> >>> - Remove superflous SIGTIMER handling on Linux __is_internal_signal >>> since it is the same of SIGCANCEL. >>> >>> - Remove dangling define and obvious comment on nptl/sigaction.c. >>> >>> Checked on x86_64-linux-gnu. >>> >>> [BZ #22391] >>> * nptl/sigaction.c (__sigaction): Use __is_internal_signal to >>> check for internal nptl signals. >>> * signal/sigaddset.c (sigaddset): Likewise. >>> * signal/sigdelset.c (sigdelset): Likewise. >>> * sysdeps/posix/signal.c (__bsd_signal): Likewise. >>> * sysdeps/posix/sigset.c (sigset): Call and check sigaddset return >>> value. >>> * signal/sigfillset.c (sigfillset): User __clear_internal_signals >>> to filter out internal nptl signals. >>> * signal/tst-sigset.c (do_test): Check ech signal indidually and >>> also check realtime signals using standard macros. >>> * sysdeps/nptl/nptl-signals.h (__clear_internal_signals, >>> __is_internal_signal): New functions. >>> * sysdeps/nptl/sigfillset.c: Remove file. >>> * sysdeps/unix/sysv/linux/nptl-signals.h (__is_internal_signal): >>> Change return to bool. >>> (__clear_internal_signals): Remove SIGTIMER clean since it is >>> equal to SIGCANEL on Linux. >>> * sysdeps/unix/sysv/linux/sigtimedwait.c (__sigtimedwait): Assume >>> signal set was constructed using standard functions. >>> * sysdeps/unix/sysv/linux/sigwait.c (do_sigtwait): Likewise. >>> >>> Signed-off-by: Adhemerval Zanella >>> Reported-by: Yury Norov >>> --- >>> ChangeLog | 23 ++++++++ >>> nptl/sigaction.c | 14 +---- >>> signal/sigaction.c | 2 +- >>> signal/sigaddset.c | 5 +- >>> signal/sigdelset.c | 5 +- >>> signal/sigfillset.c | 10 +--- >>> signal/tst-sigset.c | 92 ++++++++++++++++++++++-------- >>> sysdeps/generic/internal-signals.h | 11 ++++ >>> sysdeps/nptl/sigfillset.c | 20 ------- >>> sysdeps/posix/signal.c | 5 +- >>> sysdeps/posix/sigset.c | 10 +--- >>> sysdeps/unix/sysv/linux/internal-signals.h | 4 +- >>> sysdeps/unix/sysv/linux/sigtimedwait.c | 17 +----- >>> 13 files changed, 122 insertions(+), 96 deletions(-) >>> delete mode 100644 sysdeps/nptl/sigfillset.c >>> >>> diff --git a/nptl/sigaction.c b/nptl/sigaction.c >>> index ddf6f5e..79b6fdc 100644 >>> --- a/nptl/sigaction.c >>> +++ b/nptl/sigaction.c >>> @@ -16,22 +16,12 @@ >>> License along with the GNU C Library; if not, see >>> . */ >>> >>> - >>> -/* This is no complete implementation. The file is meant to be >>> - included in the real implementation to provide the wrapper around >>> - __libc_sigaction. */ >>> - >>> -#include >>> - >>> -/* We use the libc implementation but we tell it to not allow >>> - SIGCANCEL or SIGTIMER to be handled. */ >>> -#define LIBC_SIGACTION 1 >>> - >>> +#include >>> >>> int >>> __sigaction (int sig, const struct sigaction *act, struct sigaction *oact) >>> { >>> - if (__glibc_unlikely (sig == SIGCANCEL || sig == SIGSETXID)) >>> + if (sig <= 0 || sig >= NSIG || __is_internal_signal (sig)) >>> { >>> __set_errno (EINVAL); >>> return -1; >>> diff --git a/signal/sigaction.c b/signal/sigaction.c >>> index f761ca2..c99001a 100644 >>> --- a/signal/sigaction.c >>> +++ b/signal/sigaction.c >>> @@ -24,7 +24,7 @@ >>> int >>> __sigaction (int sig, const struct sigaction *act, struct sigaction *oact) >>> { >>> - if (sig <= 0 || sig >= NSIG) >>> + if (sig <= 0 || sig >= NSIG || __is_internal_signal (sig)) >>> { >>> __set_errno (EINVAL); >>> return -1; >>> diff --git a/signal/sigaddset.c b/signal/sigaddset.c >>> index d310890..7238df4 100644 >>> --- a/signal/sigaddset.c >>> +++ b/signal/sigaddset.c >>> @@ -17,13 +17,14 @@ >>> >>> #include >>> #include >>> -#include >>> +#include >>> >>> /* Add SIGNO to SET. */ >>> int >>> sigaddset (sigset_t *set, int signo) >>> { >>> - if (set == NULL || signo <= 0 || signo >= NSIG) >>> + if (set == NULL || signo <= 0 || signo >= NSIG >>> + || __is_internal_signal (signo)) >>> { >>> __set_errno (EINVAL); >>> return -1; >>> diff --git a/signal/sigdelset.c b/signal/sigdelset.c >>> index cd83dda..011978c 100644 >>> --- a/signal/sigdelset.c >>> +++ b/signal/sigdelset.c >>> @@ -17,13 +17,14 @@ >>> >>> #include >>> #include >>> -#include >>> +#include >>> >>> /* Add SIGNO to SET. */ >>> int >>> sigdelset (sigset_t *set, int signo) >>> { >>> - if (set == NULL || signo <= 0 || signo >= NSIG) >>> + if (set == NULL || signo <= 0 || signo >= NSIG >>> + || __is_internal_signal (signo)) >>> { >>> __set_errno (EINVAL); >>> return -1; >>> diff --git a/signal/sigfillset.c b/signal/sigfillset.c >>> index e586fd9..83dd583 100644 >>> --- a/signal/sigfillset.c >>> +++ b/signal/sigfillset.c >>> @@ -18,6 +18,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> >>> /* Set all signals in SET. */ >>> int >>> @@ -31,14 +32,7 @@ sigfillset (sigset_t *set) >>> >>> memset (set, 0xff, sizeof (sigset_t)); >>> >>> - /* If the implementation uses a cancellation signal don't set the bit. */ >>> -#ifdef SIGCANCEL >>> - __sigdelset (set, SIGCANCEL); >>> -#endif >>> - /* Likewise for the signal to implement setxid. */ >>> -#ifdef SIGSETXID >>> - __sigdelset (set, SIGSETXID); >>> -#endif >>> + __clear_internal_signals (set); >>> >>> return 0; >>> } >>> diff --git a/signal/tst-sigset.c b/signal/tst-sigset.c >>> index d47adcc..a2b764d 100644 >>> --- a/signal/tst-sigset.c >>> +++ b/signal/tst-sigset.c >>> @@ -1,43 +1,85 @@ >>> /* Test sig*set functions. */ >>> >>> #include >>> -#include >>> >>> -#define TEST_FUNCTION do_test () >>> +#include >>> + >>> static int >>> do_test (void) >>> { >>> - int result = 0; >>> - int sig = -1; >>> + sigset_t set; >>> + TEST_VERIFY (sigemptyset (&set) == 0); >>> >>> -#define TRY(call) \ >>> - if (call) \ >>> - { \ >>> - printf ("%s (sig = %d): %m\n", #call, sig); \ >>> - result = 1; \ >>> - } \ >>> - else >>> +#define VERIFY(set, sig) \ >>> + TEST_VERIFY (sigismember (&set, sig) == 0); \ >>> + TEST_VERIFY (sigaddset (&set, sig) == 0); \ >>> + TEST_VERIFY (sigismember (&set, sig) != 0); \ >>> + TEST_VERIFY (sigdelset (&set, sig) == 0); \ >>> + TEST_VERIFY (sigismember (&set, sig) == 0) >>> >>> + /* ISO C99 signals. */ >>> + VERIFY (set, SIGINT); >>> + VERIFY (set, SIGILL); >>> + VERIFY (set, SIGABRT); >>> + VERIFY (set, SIGFPE); >>> + VERIFY (set, SIGSEGV); >>> + VERIFY (set, SIGTERM); >>> >>> - sigset_t set; >>> - TRY (sigemptyset (&set) != 0); >>> + /* Historical signals specified by POSIX. */ >>> + VERIFY (set, SIGHUP); >>> + VERIFY (set, SIGQUIT); >>> + VERIFY (set, SIGTRAP); >>> + VERIFY (set, SIGKILL); >>> + VERIFY (set, SIGBUS); >>> + VERIFY (set, SIGSYS); >>> + VERIFY (set, SIGPIPE); >>> + VERIFY (set, SIGALRM); >>> + >>> + /* New(er) POSIX signals (1003.1-2008, 1003.1-2013). */ >>> + VERIFY (set, SIGURG); >>> + VERIFY (set, SIGSTOP); >>> + VERIFY (set, SIGTSTP); >>> + VERIFY (set, SIGCONT); >>> + VERIFY (set, SIGCHLD); >>> + VERIFY (set, SIGTTIN); >>> + VERIFY (set, SIGTTOU); >>> + VERIFY (set, SIGPOLL); >>> + VERIFY (set, SIGXCPU); >>> + VERIFY (set, SIGXFSZ); >>> + VERIFY (set, SIGVTALRM); >>> + VERIFY (set, SIGPROF); >>> + VERIFY (set, SIGUSR1); >>> + VERIFY (set, SIGUSR2); >>> + >>> + /* Nonstandard signals found in all modern POSIX systems >>> + (including both BSD and Linux). */ >>> + VERIFY (set, SIGWINCH); >>> >>> -#ifdef SIGRTMAX >>> - int max_sig = SIGRTMAX; >>> -#else >>> - int max_sig = NSIG - 1; >>> + /* Arch-specific signals. */ >>> +#ifdef SIGEMT >>> + VERIFY (set, SIGEMT); >>> +#endif >>> +#ifdef SIGLOST >>> + VERIFY (set, SIGLOST); >>> +#endif >>> +#ifdef SIGINFO >>> + VERIFY (set, SIGINFO); >>> +#endif >>> +#ifdef SIGSTKFLT >>> + VERIFY (set, SIGSTKFLT); >>> +#endif >>> +#ifdef SIGPWR >>> + VERIFY (set, SIGPWR); >>> #endif >>> >>> - for (sig = 1; sig <= max_sig; ++sig) >>> + /* Read-time signals (POSIX.1b real-time extensions). If they are >>> + supported SIGRTMAX value is greater than SIGRTMIN. */ >>> + for (int rtsig = SIGRTMIN; rtsig <= SIGRTMAX; rtsig++) >>> { >>> - TRY (sigismember (&set, sig) != 0); >>> - TRY (sigaddset (&set, sig) != 0); >>> - TRY (sigismember (&set, sig) == 0); >>> - TRY (sigdelset (&set, sig) != 0); >>> - TRY (sigismember (&set, sig) != 0); >>> + VERIFY (set, rtsig); >>> } >>> >>> - return result; >>> + return 0; >>> } >>> >>> -#include "../test-skeleton.c" >>> +#include >>> diff --git a/sysdeps/generic/internal-signals.h b/sysdeps/generic/internal-signals.h >>> index 01e5b75..ab0b22e 100644 >>> --- a/sysdeps/generic/internal-signals.h >>> +++ b/sysdeps/generic/internal-signals.h >>> @@ -15,3 +15,14 @@ >>> You should have received a copy of the GNU Lesser General Public >>> License along with the GNU C Library; if not, see >>> . */ >>> + >>> +static inline void >>> +__clear_internal_signals (sigset_t *set) >>> +{ >>> +} >>> + >>> +static inline bool >>> +__is_internal_signal (int sig) >>> +{ >>> + return false; >>> +} >>> diff --git a/sysdeps/nptl/sigfillset.c b/sysdeps/nptl/sigfillset.c >>> deleted file mode 100644 >>> index 94a7680..0000000 >>> --- a/sysdeps/nptl/sigfillset.c >>> +++ /dev/null >>> @@ -1,20 +0,0 @@ >>> -/* Copyright (C) 2003-2018 Free Software Foundation, Inc. >>> - This file is part of the GNU C Library. >>> - >>> - The GNU C Library is free software; you can redistribute it and/or >>> - modify it under the terms of the GNU Lesser General Public >>> - License as published by the Free Software Foundation; either >>> - version 2.1 of the License, or (at your option) any later version. >>> - >>> - The GNU C Library is distributed in the hope that it will be useful, >>> - but WITHOUT ANY WARRANTY; without even the implied warranty of >>> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >>> - Lesser General Public License for more details. >>> - >>> - You should have received a copy of the GNU Lesser General Public >>> - License along with the GNU C Library; if not, see >>> - . */ >>> - >>> -#include >>> - >>> -#include >>> diff --git a/sysdeps/posix/signal.c b/sysdeps/posix/signal.c >>> index a4a0875..8a135c7 100644 >>> --- a/sysdeps/posix/signal.c >>> +++ b/sysdeps/posix/signal.c >>> @@ -18,8 +18,8 @@ >>> >>> #include >>> #include >>> -#include /* For the real memset prototype. */ >>> #include >>> +#include >>> >>> sigset_t _sigintr attribute_hidden; /* Set by siginterrupt. */ >>> >>> @@ -31,7 +31,8 @@ __bsd_signal (int sig, __sighandler_t handler) >>> struct sigaction act, oact; >>> >>> /* Check signal extents to protect __sigismember. */ >>> - if (handler == SIG_ERR || sig < 1 || sig >= NSIG) >>> + if (handler == SIG_ERR || sig < 1 || sig >= NSIG >>> + || __is_internal_signal (sig)) >>> { >>> __set_errno (EINVAL); >>> return SIG_ERR; >>> diff --git a/sysdeps/posix/sigset.c b/sysdeps/posix/sigset.c >>> index b62aa3c..6ab4a48 100644 >>> --- a/sysdeps/posix/sigset.c >>> +++ b/sysdeps/posix/sigset.c >>> @@ -31,15 +31,9 @@ sigset (int sig, __sighandler_t disp) >>> sigset_t set; >>> sigset_t oset; >>> >>> - /* Check signal extents to protect __sigismember. */ >>> - if (disp == SIG_ERR || sig < 1 || sig >= NSIG) >>> - { >>> - __set_errno (EINVAL); >>> - return SIG_ERR; >>> - } >>> - >>> __sigemptyset (&set); >>> - __sigaddset (&set, sig); >>> + if (sigaddset (&set, sig) < 0) >>> + return SIG_ERR; >>> >>> if (disp == SIG_HOLD) >>> { >>> diff --git a/sysdeps/unix/sysv/linux/internal-signals.h b/sysdeps/unix/sysv/linux/internal-signals.h >>> index e007372..5ff4cf8 100644 >>> --- a/sysdeps/unix/sysv/linux/internal-signals.h >>> +++ b/sysdeps/unix/sysv/linux/internal-signals.h >>> @@ -21,6 +21,8 @@ >>> >>> #include >>> #include >>> +#include >>> +#include >>> >>> /* The signal used for asynchronous cancelation. */ >>> #define SIGCANCEL __SIGRTMIN >>> @@ -37,7 +39,7 @@ >>> >>> >>> /* Return is sig is used internally. */ >>> -static inline int >>> +static inline bool >>> __is_internal_signal (int sig) >>> { >>> return (sig == SIGCANCEL) || (sig == SIGSETXID); >>> diff --git a/sysdeps/unix/sysv/linux/sigtimedwait.c b/sysdeps/unix/sysv/linux/sigtimedwait.c >>> index 051a285..b4de885 100644 >>> --- a/sysdeps/unix/sysv/linux/sigtimedwait.c >>> +++ b/sysdeps/unix/sysv/linux/sigtimedwait.c >>> @@ -24,21 +24,8 @@ int >>> __sigtimedwait (const sigset_t *set, siginfo_t *info, >>> const struct timespec *timeout) >>> { >>> - sigset_t tmpset; >>> - if (set != NULL >>> - && (__builtin_expect (__sigismember (set, SIGCANCEL), 0) >>> - || __builtin_expect (__sigismember (set, SIGSETXID), 0))) >>> - { >>> - /* Create a temporary mask without the bit for SIGCANCEL set. */ >>> - // We are not copying more than we have to. >>> - memcpy (&tmpset, set, _NSIG / 8); >>> - __sigdelset (&tmpset, SIGCANCEL); >>> - __sigdelset (&tmpset, SIGSETXID); >>> - set = &tmpset; >>> - } >>> - >>> - /* XXX The size argument hopefully will have to be changed to the >>> - real size of the user-level sigset_t. */ >>> + /* XXX The size argument hopefully will have to be changed to the >>> + real size of the user-level sigset_t. */ >>> int result = SYSCALL_CANCEL (rt_sigtimedwait, set, info, timeout, _NSIG / 8); >>> >>> /* The kernel generates a SI_TKILL code in si_code in case tkill is >>>