Re: [PATCH 2/3] Consolidate Linux sigprocmask() implementation

[Yury Norov <ynorov at caviumnetworks dot com>]

Hi Adhemerval!

On Tue, Oct 31, 2017 at 07:19:01PM -0200, Adhemerval Zanella wrote:
> 
> 
> On 16/10/2017 02:34, Yury Norov wrote:
> > ia64, s390-64, sparc64, x86_64 and alpha ports has their own
> > implementations of sigprocmask(). They all but alpha do exactly
> > what generic sigprocmask() except the check and clear SIGCANCEL
> > and SIGSETXID flags.
> > 
> > In this patch, the NEED_CLEAR_SIGCANCEL_SIGSETXID option is
> > introduced and disabled for that ports which allows to swith
> > them to generic implementation.
> 
> Although the manual do not state the Linux implementation detail I think
> all supported Linux architecture should have the same semantic regarding 
> SIGCANCEL and SIGSETXID.  GLIBC on Linux requires both signal to proper
> implement both pthread cancellation and set*id function and having
> different semanticsis 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).
> 
> Also, sigfillset removes SIGCANCEL and SIGSETXID as expected, but
> sigaddset and sigdelset does not handle none of internal signals.  I also
> think we should ignore internal nptl signals on sigaddset and sigdelset.
> 
> And for this specific case I don't see adding compat symbols to keep
> the old semantic for the related architectures the best approach.  There
> is a canonical way to actually disable pthread cancellation and masking
> SIGSETXID would make set*id non POSIX conformant.
> 
> What about the following?

I suspected that sigprocmask is buggy and should be fixed as you
suggested here. Now after your explanation I'm convinced with it. But
your patch changes user interface to glibc which may break existing
software.

The most conservative way to proceed with it is to leave the existing
behavior for affected platforms as is. For software compiled against
glibc-2.27 or newer we can use versioning to wire sigprocmask to 
__new_sigprocmask, which would emit warning for x86 and others, and
clear internal signals if they appear.

But I'm not familiar with nptl, and if you think that silent API
change will not hurt users, I'm OK with your patch as is. In this
case I would only ask you to add notes about this changes to NEWS,
and especially about alpha as it is switched to new syscall.

Yury

> ---
> 
> This patch consolidates the sigprocmask Linux syscall implementation on
> sysdeps/unix/sysv/linux/sigprocmask.c.  The changes are:
> 
>   1. For ia64, s390-64, sparc64, and x86_64 the default semantic for
>      filter out SIGCANCEL and SIGSETXID is used.  Also the Linux pthread
>      semantic is documented in the signal chapter.
> 
>   2. A new internal function to check for NPTL internal signals within a
>      signal set is added (__nptl_has_internal_signal).  It is used to
>      simplify the default sigprocmask.c implementation.
> 
> Checked on x86_64-linux-gnu.
> 
> 	* manual/signal.texi: Add a note about internal pthread signals
> 	on Linux.
> 	* sysdeps/unix/sysv/linux/alpha/sigprocmask.c: Remove file.
> 	* sysdeps/unix/sysv/linux/ia64/sigprocmask.c: Likewise.
> 	* sysdeps/unix/sysv/linux/sparc/sparc64/sigprocmask.c: Likewise.
> 	* sysdeps/unix/sysv/linux/x86_64/sigprocmask.c: Likewise.
> 	* sysdeps/unix/sysv/linux/nptl-signals.h
> 	(__nptl_has_internal_signal): New function.
> 	* sysdeps/unix/sysv/linux/sigprocmask.c (__sigprocmask):
> 	Use __nptl_has_internal_signal and __nptl_clear_internal_signals
> 	function.
> 
> Signed-off-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> 
> ---
> 
> diff --git a/manual/signal.texi b/manual/signal.texi
> index 9577ff0..46696af 100644
> --- a/manual/signal.texi
> +++ b/manual/signal.texi
> @@ -2461,6 +2461,11 @@ well.  (In addition, it's not wise to put into your program an
>  assumption that the system has no signals aside from the ones you know
>  about.)
>  
> +On Linux pthreads internally uses some signals to implement asynchronous
> +cancellation, effective ID synchronization for POSIX conformance, and
> +posix timer management.  These signals are filtered out on signal set
> +function manipulation.
> +
>  @deftypefun int sigemptyset (sigset_t *@var{set})
>  @standards{POSIX.1, signal.h}
>  @safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
> diff --git a/sysdeps/unix/sysv/linux/alpha/sigprocmask.c b/sysdeps/unix/sysv/linux/alpha/sigprocmask.c
> deleted file mode 100644
> index ebec70c..0000000
> --- a/sysdeps/unix/sysv/linux/alpha/sigprocmask.c
> +++ /dev/null
> @@ -1,58 +0,0 @@
> -/* Copyright (C) 1993-2017 Free Software Foundation, Inc.
> -   This file is part of the GNU C Library.
> -   Contributed by David Mosberger (davidm@azstarnet.com).
> -
> -   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 <errno.h>
> -#include <sysdep.h>
> -#include <signal.h>
> -
> -/* When there is kernel support for more than 64 signals, we'll have to
> -   switch to a new system call convention here.  */
> -
> -int
> -__sigprocmask (int how, const sigset_t *set, sigset_t *oset)
> -{
> -  unsigned long int setval;
> -  long result;
> -
> -  if (set)
> -    setval = set->__val[0];
> -  else
> -    {
> -      setval = 0;
> -      how = SIG_BLOCK;	/* ensure blocked mask doesn't get changed */
> -    }
> -
> -  result = INLINE_SYSCALL (osf_sigprocmask, 2, how, setval);
> -  if (result == -1)
> -    /* If there are ever more than 63 signals, we need to recode this
> -       in assembler since we wouldn't be able to distinguish a mask of
> -       all 1s from -1, but for now, we're doing just fine... */
> -    return result;
> -
> -  if (oset)
> -    {
> -      oset->__val[0] = result;
> -      result = _SIGSET_NWORDS;
> -      while (--result > 0)
> -	oset->__val[result] = 0;
> -    }
> -  return 0;
> -}
> -
> -libc_hidden_def (__sigprocmask)
> -weak_alias (__sigprocmask, sigprocmask);
> diff --git a/sysdeps/unix/sysv/linux/ia64/sigprocmask.c b/sysdeps/unix/sysv/linux/ia64/sigprocmask.c
> deleted file mode 100644
> index 920c5fd..0000000
> --- a/sysdeps/unix/sysv/linux/ia64/sigprocmask.c
> +++ /dev/null
> @@ -1,40 +0,0 @@
> -/* Copyright (C) 1997-2017 Free Software Foundation, Inc.
> -   This file is part of the GNU C Library.
> -   Linux/IA64 specific sigprocmask
> -   Written by Jes Sorensen, <Jes.Sorensen@cern.ch>, April 1999.
> -
> -   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/>.  */
> -
> -/* Linux/ia64 only has rt signals, thus we do not even want to try falling
> -   back to the old style signals as the default Linux handler does. */
> -
> -#include <errno.h>
> -#include <signal.h>
> -#include <unistd.h>
> -
> -#include <sysdep.h>
> -#include <sys/syscall.h>
> -
> -/* Get and/or change the set of blocked signals.  */
> -int
> -__sigprocmask (int how, const sigset_t *set, sigset_t *oset)
> -{
> -
> -  /* XXX The size argument hopefully will have to be changed to the
> -     real size of the user-level sigset_t.  */
> -  return INLINE_SYSCALL (rt_sigprocmask, 4, how, set, oset, _NSIG / 8);
> -}
> -libc_hidden_def (__sigprocmask)
> -weak_alias (__sigprocmask, sigprocmask)
> diff --git a/sysdeps/unix/sysv/linux/nptl-signals.h b/sysdeps/unix/sysv/linux/nptl-signals.h
> index f30c597..6afd3fe 100644
> --- a/sysdeps/unix/sysv/linux/nptl-signals.h
> +++ b/sysdeps/unix/sysv/linux/nptl-signals.h
> @@ -33,6 +33,12 @@
>  #define SIGSETXID       (__SIGRTMIN + 1)
>  
>  
> +static inline bool
> +__nptl_has_internal_signal (const sigset_t *set)
> +{
> +  return __sigismember (set, SIGCANCEL) || __sigismember (set, SIGSETXID);
> +}
> +
>  /* Return is sig is used internally.  */
>  static inline int
>  __nptl_is_internal_signal (int sig)
> diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/sigprocmask.c b/sysdeps/unix/sysv/linux/s390/s390-64/sigprocmask.c
> deleted file mode 100644
> index a8010e7..0000000
> --- a/sysdeps/unix/sysv/linux/s390/s390-64/sigprocmask.c
> +++ /dev/null
> @@ -1,38 +0,0 @@
> -/* Copyright (C) 2001-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/>.  */
> -
> -/* 64 bit Linux for S/390 only has rt signals, thus we do not even want to try
> -   falling back to the old style signals as the default Linux handler does. */
> -
> -#include <errno.h>
> -#include <signal.h>
> -#include <unistd.h>
> -
> -#include <sysdep.h>
> -#include <sys/syscall.h>
> -
> -/* Get and/or change the set of blocked signals.  */
> -int
> -__sigprocmask (int how, const sigset_t *set, sigset_t *oset)
> -{
> -
> -  /* XXX The size argument hopefully will have to be changed to the
> -     real size of the user-level sigset_t.  */
> -  return INLINE_SYSCALL (rt_sigprocmask, 4, how, set, oset, _NSIG / 8);
> -}
> -libc_hidden_def (__sigprocmask)
> -weak_alias (__sigprocmask, sigprocmask)
> diff --git a/sysdeps/unix/sysv/linux/sigprocmask.c b/sysdeps/unix/sysv/linux/sigprocmask.c
> index e776563..d14fc5c 100644
> --- a/sysdeps/unix/sysv/linux/sigprocmask.c
> +++ b/sysdeps/unix/sysv/linux/sigprocmask.c
> @@ -1,4 +1,5 @@
> -/* Copyright (C) 1997-2017 Free Software Foundation, Inc.
> +/* Get and/or change the set of blocked signals.  Linux version.
> +   Copyright (C) 1997-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
> @@ -17,34 +18,22 @@
>  
>  #include <errno.h>
>  #include <signal.h>
> -#include <string.h>  /* Needed for string function builtin redirection.  */
> -#include <unistd.h>
> +#include <nptl-signals.h>
>  
> -#include <sysdep.h>
> -#include <sys/syscall.h>
>  
> -#include <nptl/pthreadP.h>              /* SIGCANCEL, SIGSETXID */
> -
> -
> -/* Get and/or change the set of blocked signals.  */
>  int
>  __sigprocmask (int how, const sigset_t *set, sigset_t *oset)
>  {
>    sigset_t local_newmask;
>  
> -  /* The only thing we have to make sure here is that SIGCANCEL and
> -     SIGSETXID are not blocked.  */
> -  if (set != NULL
> -      && (__builtin_expect (__sigismember (set, SIGCANCEL), 0)
> -	  || __builtin_expect (__sigismember (set, SIGSETXID), 0)))
> +  if (set != NULL && __glibc_unlikely (__nptl_has_internal_signal (set)))
>      {
>        local_newmask = *set;
> -      __sigdelset (&local_newmask, SIGCANCEL);
> -      __sigdelset (&local_newmask, SIGSETXID);
> +      __nptl_clear_internal_signals (&local_newmask);
>        set = &local_newmask;
>      }
>  
> -  return INLINE_SYSCALL (rt_sigprocmask, 4, how, set, oset, _NSIG / 8);
> +  return INLINE_SYSCALL_CALL (rt_sigprocmask, how, set, oset, _NSIG / 8);
>  }
>  libc_hidden_def (__sigprocmask)
>  weak_alias (__sigprocmask, sigprocmask)
> diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/sigprocmask.c b/sysdeps/unix/sysv/linux/sparc/sparc64/sigprocmask.c
> deleted file mode 100644
> index ef7d7fe..0000000
> --- a/sysdeps/unix/sysv/linux/sparc/sparc64/sigprocmask.c
> +++ /dev/null
> @@ -1,34 +0,0 @@
> -/* Copyright (C) 1997-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 <errno.h>
> -#include <signal.h>
> -#include <unistd.h>
> -
> -#include <sysdep.h>
> -#include <sys/syscall.h>
> -
> -/* Get and/or change the set of blocked signals.  */
> -int
> -__sigprocmask (int how, const sigset_t *set, sigset_t *oset)
> -{
> -  /* XXX The size argument hopefully will have to be changed to the
> -     real size of the user-level sigset_t.  */
> -  return INLINE_SYSCALL (rt_sigprocmask, 4, how, set, oset, _NSIG / 8);
> -}
> -libc_hidden_def (__sigprocmask)
> -weak_alias (__sigprocmask, sigprocmask)
> diff --git a/sysdeps/unix/sysv/linux/x86_64/sigprocmask.c b/sysdeps/unix/sysv/linux/x86_64/sigprocmask.c
> deleted file mode 100644
> index 1610ddf..0000000
> --- a/sysdeps/unix/sysv/linux/x86_64/sigprocmask.c
> +++ /dev/null
> @@ -1,39 +0,0 @@
> -/* Copyright (C) 1997-2017 Free Software Foundation, Inc.
> -   This file is part of the GNU C Library.
> -   Written by Jes Sorensen, <Jes.Sorensen@cern.ch>, April 1999.
> -
> -   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/>.  */
> -
> -/* Linux/x86_64 only has rt signals, thus we do not even want to try falling
> -   back to the old style signals as the default Linux handler does. */
> -
> -#include <errno.h>
> -#include <signal.h>
> -#include <unistd.h>
> -
> -#include <sysdep.h>
> -#include <sys/syscall.h>
> -
> -/* Get and/or change the set of blocked signals.  */
> -int
> -__sigprocmask (int how, const sigset_t *set, sigset_t *oset)
> -{
> -
> -  /* XXX The size argument hopefully will have to be changed to the
> -     real size of the user-level sigset_t.  */
> -  return INLINE_SYSCALL (rt_sigprocmask, 4, how, set, oset, _NSIG / 8);
> -}
> -libc_hidden_def (__sigprocmask)
> -weak_alias (__sigprocmask, sigprocmask)