From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 54256 invoked by alias); 19 Feb 2018 17:50:51 -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 54195 invoked by uid 89); 19 Feb 2018 17:50:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-qt0-f195.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:from:to:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=/9E4K39zLcSI2WItVjNrLKVljSo9HomoL8lWRO0O3AA=; b=kPM02Jmo6QYqd9fJDtLPWqQV+AVPJxvruSe6+utA/DjqjVqSN6pl47zM1OFn0qgryN a15dlOnJ6Y0f/wL4USuvR41VEXXAuZ0wwJIgwray3aDowausT2tZLwxQz2GA+tTIB4lK Ug870o08g3nuJWUX7hAg7eRYsICbozbsl9t4bRu0cEHNH46tOQwJrptLhrhion3lyWMU 8OioFI8Q6s15cJ9EWWqMS7JDw1NL3FtKYCOD0KvUdAYA7QDukBgNCgAdK9NC98xE/hdh ut8EN5+ZjMJy13DCcX/ugtlt4CTEzPJ6z75ipOSoVJEtc1beGa/lk1X95P5kgABvJc3y 63QA== X-Gm-Message-State: APf1xPAhsAe4xbYlIfD1eRBe2atbslAyQGvbSXzyJZ6LfTlAgbGyYOdc qpub47RsgfMW0mlfps6OHVeQFil6DRo= X-Google-Smtp-Source: AH8x2241/hHxYSI2G2c93cYViSyM9IwpO5p37yPAKxyGFk5LclpKeoES8O6N8En3QdJKC51+dACZoA== X-Received: by 10.200.25.60 with SMTP id t57mr26459826qtj.81.1519062644204; Mon, 19 Feb 2018 09:50:44 -0800 (PST) Subject: Re: [PATCH v4 2/4] Filter out NPTL internal signals (BZ #22391) From: Adhemerval Zanella To: libc-alpha@sourceware.org References: <1518439345-6013-1-git-send-email-adhemerval.zanella@linaro.org> <1518439345-6013-2-git-send-email-adhemerval.zanella@linaro.org> Message-ID: <43cc9193-1cea-6ae3-388e-88bb3406ac9c@linaro.org> Date: Mon, 19 Feb 2018 17:51:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <1518439345-6013-2-git-send-email-adhemerval.zanella@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-02/txt/msg00515.txt.bz2 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 >