Re: [PATCH v4 2/4] Filter out NPTL internal signals (BZ #22391)

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

Right, I will fix it.

On 03/04/2018 12:12, Zack Weinberg wrote:
> 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
> <adhemerval.zanella@linaro.org> 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 <adhemerval.zanella@linaro.org>
>>>> Reported-by: Yury Norov <ynorov@caviumnetworks.com>
>>>> ---
>>>>  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
>>>>     <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 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 <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 cd83dda..011978c 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 e586fd9..83dd583 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 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
>>>>     <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 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
>>>> -   <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 a4a0875..8a135c7 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 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 <signal.h>
>>>>  #include <sigsetops.h>
>>>> +#include <stdbool.h>
>>>> +#include <sysdep.h>
>>>>
>>>>  /* 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
>>>>