This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v3 3/3] Filter out NPTL internal signals (BZ #22391)
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: libc-alpha at sourceware dot org
- Date: Mon, 1 Jan 2018 19:56:38 -0200
- Subject: Re: [PATCH v3 3/3] Filter out NPTL internal signals (BZ #22391)
- Authentication-results: sourceware.org; auth=none
- References: <1510768451-18453-1-git-send-email-adhemerval.zanella@linaro.org> <1510768451-18453-3-git-send-email-adhemerval.zanella@linaro.org> <1c09aee4-c732-f617-02c5-c64d96ed3140@linaro.org>
Ping.
On 12/12/2017 15:03, Adhemerval Zanella wrote:
> Ping.
>
> On 15/11/2017 15:54, 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 <adhemerval.zanella@linaro.org>
>> Reported-by: Yury Norov <ynorov@caviumnetworks.com>
>> ---
>> ChangeLog | 25 ++++++++
>> 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, 124 insertions(+), 96 deletions(-)
>> delete mode 100644 sysdeps/nptl/sigfillset.c
>>
>> diff --git a/nptl/sigaction.c b/nptl/sigaction.c
>> index 2994fd5..b2ff674 100644
>> --- a/nptl/sigaction.c
>> +++ b/nptl/sigaction.c
>> @@ -16,22 +16,12 @@
>> License along with the GNU C Library; if not, see
>> <http://www.gnu.org/licenses/>. */
>>
>> -
>> -/* This is no complete implementation. The file is meant to be
>> - included in the real implementation to provide the wrapper around
>> - __libc_sigaction. */
>> -
>> -#include <nptl/pthreadP.h>
>> -
>> -/* We use the libc implementation but we tell it to not allow
>> - SIGCANCEL or SIGTIMER to be handled. */
>> -#define LIBC_SIGACTION 1
>> -
>> +#include <internal-signals.h>
>>
>> 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 8a6220c..3025aab 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 161be7b..a435f61 100644
>> --- a/signal/sigaddset.c
>> +++ b/signal/sigaddset.c
>> @@ -17,13 +17,14 @@
>>
>> #include <errno.h>
>> #include <signal.h>
>> -#include <sigsetops.h>
>> +#include <internal-signals.h>
>>
>> /* 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 2aaa536..01a50ec 100644
>> --- a/signal/sigdelset.c
>> +++ b/signal/sigdelset.c
>> @@ -17,13 +17,14 @@
>>
>> #include <errno.h>
>> #include <signal.h>
>> -#include <sigsetops.h>
>> +#include <internal-signals.h>
>>
>> /* 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 0fcc24a..560c66e 100644
>> --- a/signal/sigfillset.c
>> +++ b/signal/sigfillset.c
>> @@ -18,6 +18,7 @@
>> #include <errno.h>
>> #include <signal.h>
>> #include <string.h>
>> +#include <internal-signals.h>
>>
>> /* 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 <signal.h>
>> -#include <stdio.h>
>>
>> -#define TEST_FUNCTION do_test ()
>> +#include <support/check.h>
>> +
>> 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 <support/test-driver.c>
>> diff --git a/sysdeps/generic/internal-signals.h b/sysdeps/generic/internal-signals.h
>> index 55bc07d..8a8854c 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
>> <http://www.gnu.org/licenses/>. */
>> +
>> +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 50e8512..0000000
>> --- a/sysdeps/nptl/sigfillset.c
>> +++ /dev/null
>> @@ -1,20 +0,0 @@
>> -/* Copyright (C) 2003-2017 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
>> - <http://www.gnu.org/licenses/>. */
>> -
>> -#include <nptl/pthreadP.h>
>> -
>> -#include <signal/sigfillset.c>
>> diff --git a/sysdeps/posix/signal.c b/sysdeps/posix/signal.c
>> index 81ba177..87c5d1c 100644
>> --- a/sysdeps/posix/signal.c
>> +++ b/sysdeps/posix/signal.c
>> @@ -18,8 +18,8 @@
>>
>> #include <errno.h>
>> #include <signal.h>
>> -#include <string.h> /* For the real memset prototype. */
>> #include <sigsetops.h>
>> +#include <internal-signals.h>
>>
>> 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 a4dfe0a..6234ecf 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 bb8234d..bf33bbc 100644
>> --- a/sysdeps/unix/sysv/linux/internal-signals.h
>> +++ b/sysdeps/unix/sysv/linux/internal-signals.h
>> @@ -21,6 +21,8 @@
>>
>> #include <signal.h>
>> #include <sigsetops.h>
>> +#include <stdbool.h>
>> +#include <sysdep.h>
>>
>> /* The signal used for asynchronous cancelation. */
>> #define SIGCANCEL __SIGRTMIN
>> @@ -43,7 +45,7 @@ __has_internal_signal (const sigset_t *set)
>> }
>>
>> /* 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 2d96dd4..b4977e3 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
>>