[2/3] Consolidate Linux sigprocmask() implementation
diff mbox series

Message ID 20171016043407.1142-3-ynorov@caviumnetworks.com
State New
Headers show
Series
  • Cleanup and consolidate sigprocmask() for Linux
Related show

Commit Message

Yury Norov Oct. 16, 2017, 4:34 a.m. UTC
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.

	* sysdeps/unix/sysv/linux/sigprocmask.c: Introduce
	NEED_CLEAR_SIGCANCEL_SIGSETXID option for consolidation
	of Linux sigprocmask() implementation;
	* sysdeps/unix/sysv/linux/ia64/sigprocmask.c: Consolidate
	sigprocmask() implementation;
	* sysdeps/unix/sysv/linux/s390/s390-64/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/ia64/sigprocmask.c         | 22 ++--------------------
 sysdeps/unix/sysv/linux/s390/s390-64/sigprocmask.c | 22 ++--------------------
 sysdeps/unix/sysv/linux/sigprocmask.c              | 11 +++++++++--
 .../unix/sysv/linux/sparc/sparc64/sigprocmask.c    | 18 ++----------------
 sysdeps/unix/sysv/linux/x86_64/sigprocmask.c       | 22 ++--------------------
 5 files changed, 17 insertions(+), 78 deletions(-)

Comments

Adhemerval Zanella Oct. 31, 2017, 9:19 p.m. UTC | #1
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?

---

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)
Yury Norov Oct. 31, 2017, 10:58 p.m. UTC | #2
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)
Adhemerval Zanella Nov. 3, 2017, 11:19 a.m. UTC | #3
On 31/10/2017 20:58, Yury Norov wrote:
> 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

I think the main problem of providing a compat symbol is besides 
interfering with both pthread cancellation and posix timers (a explicit
conformance break), not filtering out NPTL internal signals for set*id 
programs might be a security issue where the user/group id is not 
synchronized over the threads as expected by a POSIX standard.

I have opened BZ#22391 [1] to track this issue. I am also preparing
a patch set to fix this over the signal implementations.

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=22391

Patch
diff mbox series

diff --git a/sysdeps/unix/sysv/linux/ia64/sigprocmask.c b/sysdeps/unix/sysv/linux/ia64/sigprocmask.c
index 920c5fd052..9e95e52d58 100644
--- a/sysdeps/unix/sysv/linux/ia64/sigprocmask.c
+++ b/sysdeps/unix/sysv/linux/ia64/sigprocmask.c
@@ -17,24 +17,6 @@ 
    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. */
+#define NEED_CLEAR_SIGCANCEL_SIGSETXID        0
 
-#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)
+#include <sysdeps/unix/sysv/linux/sigprocmask.c>
diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/sigprocmask.c b/sysdeps/unix/sysv/linux/s390/s390-64/sigprocmask.c
index a8010e7f14..227b512910 100644
--- a/sysdeps/unix/sysv/linux/s390/s390-64/sigprocmask.c
+++ b/sysdeps/unix/sysv/linux/s390/s390-64/sigprocmask.c
@@ -15,24 +15,6 @@ 
    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. */
+#define NEED_CLEAR_SIGCANCEL_SIGSETXID        0
 
-#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)
+#include <sysdeps/unix/sysv/linux/sigprocmask.c>
diff --git a/sysdeps/unix/sysv/linux/sigprocmask.c b/sysdeps/unix/sysv/linux/sigprocmask.c
index e776563336..94d4d0945f 100644
--- a/sysdeps/unix/sysv/linux/sigprocmask.c
+++ b/sysdeps/unix/sysv/linux/sigprocmask.c
@@ -17,19 +17,25 @@ 
 
 #include <errno.h>
 #include <signal.h>
-#include <string.h>  /* Needed for string function builtin redirection.  */
 #include <unistd.h>
 
 #include <sysdep.h>
 #include <sys/syscall.h>
 
-#include <nptl/pthreadP.h>              /* SIGCANCEL, SIGSETXID */
+#ifndef NEED_CLEAR_SIGCANCEL_SIGSETXID
+#define NEED_CLEAR_SIGCANCEL_SIGSETXID	1
+#endif
 
+#if NEED_CLEAR_SIGCANCEL_SIGSETXID
+#include <nptl/pthreadP.h>
+#include <string.h>
+#endif
 
 /* Get and/or change the set of blocked signals.  */
 int
 __sigprocmask (int how, const sigset_t *set, sigset_t *oset)
 {
+#if NEED_CLEAR_SIGCANCEL_SIGSETXID
   sigset_t local_newmask;
 
   /* The only thing we have to make sure here is that SIGCANCEL and
@@ -43,6 +49,7 @@  __sigprocmask (int how, const sigset_t *set, sigset_t *oset)
       __sigdelset (&local_newmask, SIGSETXID);
       set = &local_newmask;
     }
+#endif
 
   return INLINE_SYSCALL (rt_sigprocmask, 4, how, set, oset, _NSIG / 8);
 }
diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/sigprocmask.c b/sysdeps/unix/sysv/linux/sparc/sparc64/sigprocmask.c
index ef7d7feb49..0db24d6fce 100644
--- a/sysdeps/unix/sysv/linux/sparc/sparc64/sigprocmask.c
+++ b/sysdeps/unix/sysv/linux/sparc/sparc64/sigprocmask.c
@@ -15,20 +15,6 @@ 
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
-#include <signal.h>
-#include <unistd.h>
+#define NEED_CLEAR_SIGCANCEL_SIGSETXID        0
 
-#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)
+#include <sysdeps/unix/sysv/linux/sigprocmask.c>
diff --git a/sysdeps/unix/sysv/linux/x86_64/sigprocmask.c b/sysdeps/unix/sysv/linux/x86_64/sigprocmask.c
index 1610ddf47f..d3179ebea5 100644
--- a/sysdeps/unix/sysv/linux/x86_64/sigprocmask.c
+++ b/sysdeps/unix/sysv/linux/x86_64/sigprocmask.c
@@ -16,24 +16,6 @@ 
    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. */
+#define NEED_CLEAR_SIGCANCEL_SIGSETXID        0
 
-#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)
+#include <sysdeps/unix/sysv/linux/sigprocmask.c>