[PATCH] Refactor Linux raise implementation (BZ#15368)

[PATCH] Refactor Linux raise implementation (BZ#15368)

[Adhemerval Zanella]

This patch changes both the nptl and libc Linux raise implementation
to avoid the issues described in BZ#15368.  The strategy used is
summarized in bug report first comment:

 1. Block all signals (including internal NPTL ones);
 2. Get pid and tid directly from syscall (not relying on cached
    values);
 3. Call tgkill;
 4. Restore old signal mask.

Tested on x86_64 and i686.

	* sysdeps/unix/sysv/linux/nptl-signals.h
	(__nptl_clear_internal_signals): New function.
	(__libc_signal_block_all): Likewise.
	(__libc_signal_block_app): Likewise.
	(__libc_signal_restore_set): Likewise.
	* sysdeps/unix/sysv/linux/pt-raise.c (raise): Use Linux raise.c
	implementation.
	* sysdeps/unix/sysv/linux/raise.c (raise): Reimplement to not use
	the cached pid/tid value in pthread structure.
---
 ChangeLog                              | 12 +++++++++
 sysdeps/unix/sysv/linux/nptl-signals.h | 37 ++++++++++++++++++++++++++++
 sysdeps/unix/sysv/linux/pt-raise.c     | 23 +++--------------
 sysdeps/unix/sysv/linux/raise.c        | 45 ++++++++++++----------------------
 4 files changed, 67 insertions(+), 50 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/nptl-signals.h b/sysdeps/unix/sysv/linux/nptl-signals.h
index 01f34c2..88a0a32 100644
--- a/sysdeps/unix/sysv/linux/nptl-signals.h
+++ b/sysdeps/unix/sysv/linux/nptl-signals.h
@@ -39,5 +39,42 @@ __nptl_is_internal_signal (int sig)
   return (sig == SIGCANCEL) || (sig == SIGTIMER) || (sig == SIGSETXID);
 }
 
+static inline void
+__nptl_clear_internal_signals (sigset_t *set)
+{
+  __sigdelset (set, SIGCANCEL);
+  __sigdelset (set, SIGTIMER);
+  __sigdelset (set, SIGSETXID);
+}
+
+#define SIGALL_SET \
+  ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })
+
+static inline int
+__libc_signal_block_all (const sigset_t *set)
+{
+  INTERNAL_SYSCALL_DECL (err);
+  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &SIGALL_SET,
+			   set, _NSIG / 8);
+}
+
+static inline int
+__libc_signal_block_app (const sigset_t *set)
+{
+  sigset_t allset = SIGALL_SET;
+  __nptl_clear_internal_signals (&allset);
+  INTERNAL_SYSCALL_DECL (err);
+  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &allset, set,
+			   _NSIG / 8);
+}
+
+static inline int
+__libc_signal_restore_set (const sigset_t *set)
+{
+  INTERNAL_SYSCALL_DECL (err);
+  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_SETMASK, set, NULL,
+			   _NSIG / 8);
+}
+
 /* Used to communicate with signal handler.  */
 extern struct xid_command *__xidcmd attribute_hidden;
diff --git a/sysdeps/unix/sysv/linux/pt-raise.c b/sysdeps/unix/sysv/linux/pt-raise.c
index 715bbe9..5f6dea1 100644
--- a/sysdeps/unix/sysv/linux/pt-raise.c
+++ b/sysdeps/unix/sysv/linux/pt-raise.c
@@ -1,4 +1,5 @@
-/* Copyright (C) 2002-2016 Free Software Foundation, Inc.
+/* ISO C raise function for libpthread.
+   Copyright (C) 2002-2016 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
 
@@ -16,22 +17,4 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
-#include <signal.h>
-#include <sysdep.h>
-#include <tls.h>
-
-
-int
-raise (int sig)
-{
-  /* raise is an async-safe function.  It could be called while the
-     fork function temporarily invalidated the PID field.  Adjust for
-     that.  */
-  pid_t pid = THREAD_GETMEM (THREAD_SELF, pid);
-  if (__glibc_unlikely (pid < 0))
-    pid = -pid;
-
-  return INLINE_SYSCALL (tgkill, 3, pid, THREAD_GETMEM (THREAD_SELF, tid),
-			 sig);
-}
+#include <sysdeps/unix/sysv/linux/raise.c>
diff --git a/sysdeps/unix/sysv/linux/raise.c b/sysdeps/unix/sysv/linux/raise.c
index 3795e6e..c6ecd66 100644
--- a/sysdeps/unix/sysv/linux/raise.c
+++ b/sysdeps/unix/sysv/linux/raise.c
@@ -16,42 +16,27 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
-#include <limits.h>
 #include <signal.h>
 #include <sysdep.h>
-#include <nptl/pthreadP.h>
-
+#include <errno.h>
+#include <sys/types.h>
+#include <unistd.h>
+#include <nptl-signals.h>
 
 int
 raise (int sig)
 {
-  struct pthread *pd = THREAD_SELF;
-  pid_t pid = THREAD_GETMEM (pd, pid);
-  pid_t selftid = THREAD_GETMEM (pd, tid);
-  if (selftid == 0)
-    {
-      /* This system call is not supposed to fail.  */
-#ifdef INTERNAL_SYSCALL
-      INTERNAL_SYSCALL_DECL (err);
-      selftid = INTERNAL_SYSCALL (gettid, err, 0);
-#else
-      selftid = INLINE_SYSCALL (gettid, 0);
-#endif
-      THREAD_SETMEM (pd, tid, selftid);
-
-      /* We do not set the PID field in the TID here since we might be
-	 called from a signal handler while the thread executes fork.  */
-      pid = selftid;
-    }
-  else
-    /* raise is an async-safe function.  It could be called while the
-       fork/vfork function temporarily invalidated the PID field.  Adjust for
-       that.  */
-    if (__glibc_unlikely (pid <= 0))
-      pid = (pid & INT_MAX) == 0 ? selftid : -pid;
-
-  return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
+  sigset_t set;
+  __libc_signal_block_app (&set);
+
+  pid_t pid = __getpid ();
+  pid_t tid = INLINE_SYSCALL (gettid, 0);
+
+  int ret = INLINE_SYSCALL (tgkill, 3, pid, tid, sig);
+
+  __libc_signal_restore_set (&set);
+
+  return ret;
 }
 libc_hidden_def (raise)
 weak_alias (raise, gsignal)
-- 
2.7.4

Re: [PATCH] Refactor Linux raise implementation (BZ#15368)

[Zack Weinberg]

On Fri, Jun 17, 2016 at 2:43 PM, Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
> This patch changes both the nptl and libc Linux raise implementation
> to avoid the issues described in BZ#15368.

It would be nice to have comments in these files which explain why it
is necessary to block all signals and why the cached values are not
safe to use.  The old comment that you deleted ("raise is async-safe
and could be called while fork/vfork temporarily invalidated the PID")
would be a good start.

> The strategy used is
> summarized in bug report first comment:
>
>  1. Block all signals (including internal NPTL ones);

The code appears to block all signals *except* the internal NPTL ones.
If I understand Rich's description of the problem correctly, that is
wrong.

> +static inline int
> +__libc_signal_block_all (const sigset_t *set)
> +{

Type-safety error here: the 'set' argument is written to and should
not be 'const'.  It compiles because INTERNAL_SYSCALL() erases types,
but the compiler would be entitled to assume that 'set' is unchanged
after the call.

> +static inline int
> +__libc_signal_block_app (const sigset_t *set)

Same type-safety error here.

> +  sigset_t set;
> +  __libc_signal_block_app (&set);
> +
> +  pid_t pid = __getpid ();

If I'm reading the code correctly, __getpid does *not* bypass the cache.

> +  pid_t tid = INLINE_SYSCALL (gettid, 0);

INTERNAL_SYSCALL() here too?  gettid() can't fail, so there's no point
generating the code to potentially set errno.

zw

Re: [PATCH] Refactor Linux raise implementation (BZ#15368)

[Adhemerval Zanella]

Thanks for reviews, comment below:

On 18/06/2016 11:09, Zack Weinberg wrote:
> On Fri, Jun 17, 2016 at 2:43 PM, Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>> This patch changes both the nptl and libc Linux raise implementation
>> to avoid the issues described in BZ#15368.
> 
> It would be nice to have comments in these files which explain why it
> is necessary to block all signals and why the cached values are not
> safe to use.  The old comment that you deleted ("raise is async-safe
> and could be called while fork/vfork temporarily invalidated the PID")
> would be a good start.
> 

Indeed, I did not remove them in this new version below. I also extended
the comment a bit explaining why to block the application signals.

>> The strategy used is
>> summarized in bug report first comment:
>>
>>  1. Block all signals (including internal NPTL ones);
> 
> The code appears to block all signals *except* the internal NPTL ones.
> If I understand Rich's description of the problem correctly, that is
> wrong.

Right, we need to block only user defined handler that might fork/vfork.
The call itself is OK, but I will change the comments describing why there
is no need to block GLIBC internal handlers.

> 
>> +static inline int
>> +__libc_signal_block_all (const sigset_t *set)
>> +{
> 
> Type-safety error here: the 'set' argument is written to and should
> not be 'const'.  It compiles because INTERNAL_SYSCALL() erases types,
> but the compiler would be entitled to assume that 'set' is unchanged
> after the call.

Ack.

> 
>> +static inline int
>> +__libc_signal_block_app (const sigset_t *set)
> 
> Same type-safety error here.

Ack.

> 
>> +  sigset_t set;
>> +  __libc_signal_block_app (&set);
>> +
>> +  pid_t pid = __getpid ();
> 
> If I'm reading the code correctly, __getpid does *not* bypass the cache.
> 
>> +  pid_t tid = INLINE_SYSCALL (gettid, 0);
> 
> INTERNAL_SYSCALL() here too?  gettid() can't fail, so there's no point
> generating the code to potentially set errno.
> 

Right, I will use INTERNAL_SYSCALL.

> zw
> 

I think this version addresses all the issues you raised:

--

	* sysdeps/unix/sysv/linux/nptl-signals.h
	(__nptl_clear_internal_signals): New function.
	(__libc_signal_block_all): Likewise.
	(__libc_signal_block_app): Likewise.
	(__libc_signal_restore_set): Likewise.
	* sysdeps/unix/sysv/linux/pt-raise.c (raise): Use Linux raise.c
	implementation.
	* sysdeps/unix/sysv/linux/raise.c (raise): Reimplement to not use
	the cached pid/tid value in pthread structure.

--

diff --git a/sysdeps/unix/sysv/linux/nptl-signals.h b/sysdeps/unix/sysv/linux/nptl-signals.h
index 01f34c2..6525b6d 100644
--- a/sysdeps/unix/sysv/linux/nptl-signals.h
+++ b/sysdeps/unix/sysv/linux/nptl-signals.h
@@ -39,5 +39,42 @@ __nptl_is_internal_signal (int sig)
   return (sig == SIGCANCEL) || (sig == SIGTIMER) || (sig == SIGSETXID);
 }
 
+static inline void
+__nptl_clear_internal_signals (sigset_t *set)
+{
+  __sigdelset (set, SIGCANCEL);
+  __sigdelset (set, SIGTIMER);
+  __sigdelset (set, SIGSETXID);
+}
+
+#define SIGALL_SET \
+  ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })
+
+static inline int
+__libc_signal_block_all (sigset_t *set)
+{
+  INTERNAL_SYSCALL_DECL (err);
+  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &SIGALL_SET,
+			   set, _NSIG / 8);
+}
+
+static inline int
+__libc_signal_block_app (sigset_t *set)
+{
+  sigset_t allset = SIGALL_SET;
+  __nptl_clear_internal_signals (&allset);
+  INTERNAL_SYSCALL_DECL (err);
+  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &allset, set,
+			   _NSIG / 8);
+}
+
+static inline int
+__libc_signal_restore_set (const sigset_t *set)
+{
+  INTERNAL_SYSCALL_DECL (err);
+  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_SETMASK, set, NULL,
+			   _NSIG / 8);
+}
+
 /* Used to communicate with signal handler.  */
 extern struct xid_command *__xidcmd attribute_hidden;
diff --git a/sysdeps/unix/sysv/linux/pt-raise.c b/sysdeps/unix/sysv/linux/pt-raise.c
index 715bbe9..5f6dea1 100644
--- a/sysdeps/unix/sysv/linux/pt-raise.c
+++ b/sysdeps/unix/sysv/linux/pt-raise.c
@@ -1,4 +1,5 @@
-/* Copyright (C) 2002-2016 Free Software Foundation, Inc.
+/* ISO C raise function for libpthread.
+   Copyright (C) 2002-2016 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
 
@@ -16,22 +17,4 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
-#include <signal.h>
-#include <sysdep.h>
-#include <tls.h>
-
-
-int
-raise (int sig)
-{
-  /* raise is an async-safe function.  It could be called while the
-     fork function temporarily invalidated the PID field.  Adjust for
-     that.  */
-  pid_t pid = THREAD_GETMEM (THREAD_SELF, pid);
-  if (__glibc_unlikely (pid < 0))
-    pid = -pid;
-
-  return INLINE_SYSCALL (tgkill, 3, pid, THREAD_GETMEM (THREAD_SELF, tid),
-			 sig);
-}
+#include <sysdeps/unix/sysv/linux/raise.c>
diff --git a/sysdeps/unix/sysv/linux/raise.c b/sysdeps/unix/sysv/linux/raise.c
index 3795e6e..d386426 100644
--- a/sysdeps/unix/sysv/linux/raise.c
+++ b/sysdeps/unix/sysv/linux/raise.c
@@ -16,42 +16,34 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
-#include <limits.h>
 #include <signal.h>
 #include <sysdep.h>
-#include <nptl/pthreadP.h>
-
+#include <errno.h>
+#include <sys/types.h>
+#include <unistd.h>
+#include <nptl-signals.h>
 
 int
 raise (int sig)
 {
-  struct pthread *pd = THREAD_SELF;
-  pid_t pid = THREAD_GETMEM (pd, pid);
-  pid_t selftid = THREAD_GETMEM (pd, tid);
-  if (selftid == 0)
-    {
-      /* This system call is not supposed to fail.  */
-#ifdef INTERNAL_SYSCALL
-      INTERNAL_SYSCALL_DECL (err);
-      selftid = INTERNAL_SYSCALL (gettid, err, 0);
-#else
-      selftid = INLINE_SYSCALL (gettid, 0);
-#endif
-      THREAD_SETMEM (pd, tid, selftid);
-
-      /* We do not set the PID field in the TID here since we might be
-	 called from a signal handler while the thread executes fork.  */
-      pid = selftid;
-    }
-  else
-    /* raise is an async-safe function.  It could be called while the
-       fork/vfork function temporarily invalidated the PID field.  Adjust for
-       that.  */
-    if (__glibc_unlikely (pid <= 0))
-      pid = (pid & INT_MAX) == 0 ? selftid : -pid;
-
-  return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
+  /* raise is an async-safe function so it could be called while the
+     fork/vfork function temporarily invalidated the PID field.  To avoid
+     relying in the cached value we block all user-defined signal handler
+     (which might call fork/vfork) and issues the getpid and gettid
+     directly.  */
+
+  sigset_t set;
+  __libc_signal_block_app (&set);
+
+  INTERNAL_SYSCALL_DECL (err);
+  pid_t pid = INTERNAL_SYSCALL (getpid, err, 0);
+  pid_t tid = INTERNAL_SYSCALL (gettid, err, 0);
+
+  int ret = INLINE_SYSCALL (tgkill, 3, pid, tid, sig);
+
+  __libc_signal_restore_set (&set);
+
+  return ret;
 }
 libc_hidden_def (raise)
 weak_alias (raise, gsignal)

Re: [PATCH] Refactor Linux raise implementation (BZ#15368)

[Adhemerval Zanella]

Ping.

On 18/06/2016 14:25, Adhemerval Zanella wrote:
> Thanks for reviews, comment below:
> 
> On 18/06/2016 11:09, Zack Weinberg wrote:
>> On Fri, Jun 17, 2016 at 2:43 PM, Adhemerval Zanella
>> <adhemerval.zanella@linaro.org> wrote:
>>> This patch changes both the nptl and libc Linux raise implementation
>>> to avoid the issues described in BZ#15368.
>>
>> It would be nice to have comments in these files which explain why it
>> is necessary to block all signals and why the cached values are not
>> safe to use.  The old comment that you deleted ("raise is async-safe
>> and could be called while fork/vfork temporarily invalidated the PID")
>> would be a good start.
>>
> 
> Indeed, I did not remove them in this new version below. I also extended
> the comment a bit explaining why to block the application signals.
> 
>>> The strategy used is
>>> summarized in bug report first comment:
>>>
>>>  1. Block all signals (including internal NPTL ones);
>>
>> The code appears to block all signals *except* the internal NPTL ones.
>> If I understand Rich's description of the problem correctly, that is
>> wrong.
> 
> Right, we need to block only user defined handler that might fork/vfork.
> The call itself is OK, but I will change the comments describing why there
> is no need to block GLIBC internal handlers.
> 
>>
>>> +static inline int
>>> +__libc_signal_block_all (const sigset_t *set)
>>> +{
>>
>> Type-safety error here: the 'set' argument is written to and should
>> not be 'const'.  It compiles because INTERNAL_SYSCALL() erases types,
>> but the compiler would be entitled to assume that 'set' is unchanged
>> after the call.
> 
> Ack.
> 
>>
>>> +static inline int
>>> +__libc_signal_block_app (const sigset_t *set)
>>
>> Same type-safety error here.
> 
> Ack.
> 
>>
>>> +  sigset_t set;
>>> +  __libc_signal_block_app (&set);
>>> +
>>> +  pid_t pid = __getpid ();
>>
>> If I'm reading the code correctly, __getpid does *not* bypass the cache.
>>
>>> +  pid_t tid = INLINE_SYSCALL (gettid, 0);
>>
>> INTERNAL_SYSCALL() here too?  gettid() can't fail, so there's no point
>> generating the code to potentially set errno.
>>
> 
> Right, I will use INTERNAL_SYSCALL.
> 
>> zw
>>
> 
> I think this version addresses all the issues you raised:
> 
> --
> 
> 	* sysdeps/unix/sysv/linux/nptl-signals.h
> 	(__nptl_clear_internal_signals): New function.
> 	(__libc_signal_block_all): Likewise.
> 	(__libc_signal_block_app): Likewise.
> 	(__libc_signal_restore_set): Likewise.
> 	* sysdeps/unix/sysv/linux/pt-raise.c (raise): Use Linux raise.c
> 	implementation.
> 	* sysdeps/unix/sysv/linux/raise.c (raise): Reimplement to not use
> 	the cached pid/tid value in pthread structure.
> 
> --
> 
> diff --git a/sysdeps/unix/sysv/linux/nptl-signals.h b/sysdeps/unix/sysv/linux/nptl-signals.h
> index 01f34c2..6525b6d 100644
> --- a/sysdeps/unix/sysv/linux/nptl-signals.h
> +++ b/sysdeps/unix/sysv/linux/nptl-signals.h
> @@ -39,5 +39,42 @@ __nptl_is_internal_signal (int sig)
>    return (sig == SIGCANCEL) || (sig == SIGTIMER) || (sig == SIGSETXID);
>  }
>  
> +static inline void
> +__nptl_clear_internal_signals (sigset_t *set)
> +{
> +  __sigdelset (set, SIGCANCEL);
> +  __sigdelset (set, SIGTIMER);
> +  __sigdelset (set, SIGSETXID);
> +}
> +
> +#define SIGALL_SET \
> +  ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })
> +
> +static inline int
> +__libc_signal_block_all (sigset_t *set)
> +{
> +  INTERNAL_SYSCALL_DECL (err);
> +  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &SIGALL_SET,
> +			   set, _NSIG / 8);
> +}
> +
> +static inline int
> +__libc_signal_block_app (sigset_t *set)
> +{
> +  sigset_t allset = SIGALL_SET;
> +  __nptl_clear_internal_signals (&allset);
> +  INTERNAL_SYSCALL_DECL (err);
> +  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &allset, set,
> +			   _NSIG / 8);
> +}
> +
> +static inline int
> +__libc_signal_restore_set (const sigset_t *set)
> +{
> +  INTERNAL_SYSCALL_DECL (err);
> +  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_SETMASK, set, NULL,
> +			   _NSIG / 8);
> +}
> +
>  /* Used to communicate with signal handler.  */
>  extern struct xid_command *__xidcmd attribute_hidden;
> diff --git a/sysdeps/unix/sysv/linux/pt-raise.c b/sysdeps/unix/sysv/linux/pt-raise.c
> index 715bbe9..5f6dea1 100644
> --- a/sysdeps/unix/sysv/linux/pt-raise.c
> +++ b/sysdeps/unix/sysv/linux/pt-raise.c
> @@ -1,4 +1,5 @@
> -/* Copyright (C) 2002-2016 Free Software Foundation, Inc.
> +/* ISO C raise function for libpthread.
> +   Copyright (C) 2002-2016 Free Software Foundation, Inc.
>     This file is part of the GNU C Library.
>     Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
>  
> @@ -16,22 +17,4 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> -#include <errno.h>
> -#include <signal.h>
> -#include <sysdep.h>
> -#include <tls.h>
> -
> -
> -int
> -raise (int sig)
> -{
> -  /* raise is an async-safe function.  It could be called while the
> -     fork function temporarily invalidated the PID field.  Adjust for
> -     that.  */
> -  pid_t pid = THREAD_GETMEM (THREAD_SELF, pid);
> -  if (__glibc_unlikely (pid < 0))
> -    pid = -pid;
> -
> -  return INLINE_SYSCALL (tgkill, 3, pid, THREAD_GETMEM (THREAD_SELF, tid),
> -			 sig);
> -}
> +#include <sysdeps/unix/sysv/linux/raise.c>
> diff --git a/sysdeps/unix/sysv/linux/raise.c b/sysdeps/unix/sysv/linux/raise.c
> index 3795e6e..d386426 100644
> --- a/sysdeps/unix/sysv/linux/raise.c
> +++ b/sysdeps/unix/sysv/linux/raise.c
> @@ -16,42 +16,34 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> -#include <errno.h>
> -#include <limits.h>
>  #include <signal.h>
>  #include <sysdep.h>
> -#include <nptl/pthreadP.h>
> -
> +#include <errno.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +#include <nptl-signals.h>
>  
>  int
>  raise (int sig)
>  {
> -  struct pthread *pd = THREAD_SELF;
> -  pid_t pid = THREAD_GETMEM (pd, pid);
> -  pid_t selftid = THREAD_GETMEM (pd, tid);
> -  if (selftid == 0)
> -    {
> -      /* This system call is not supposed to fail.  */
> -#ifdef INTERNAL_SYSCALL
> -      INTERNAL_SYSCALL_DECL (err);
> -      selftid = INTERNAL_SYSCALL (gettid, err, 0);
> -#else
> -      selftid = INLINE_SYSCALL (gettid, 0);
> -#endif
> -      THREAD_SETMEM (pd, tid, selftid);
> -
> -      /* We do not set the PID field in the TID here since we might be
> -	 called from a signal handler while the thread executes fork.  */
> -      pid = selftid;
> -    }
> -  else
> -    /* raise is an async-safe function.  It could be called while the
> -       fork/vfork function temporarily invalidated the PID field.  Adjust for
> -       that.  */
> -    if (__glibc_unlikely (pid <= 0))
> -      pid = (pid & INT_MAX) == 0 ? selftid : -pid;
> -
> -  return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
> +  /* raise is an async-safe function so it could be called while the
> +     fork/vfork function temporarily invalidated the PID field.  To avoid
> +     relying in the cached value we block all user-defined signal handler
> +     (which might call fork/vfork) and issues the getpid and gettid
> +     directly.  */
> +
> +  sigset_t set;
> +  __libc_signal_block_app (&set);
> +
> +  INTERNAL_SYSCALL_DECL (err);
> +  pid_t pid = INTERNAL_SYSCALL (getpid, err, 0);
> +  pid_t tid = INTERNAL_SYSCALL (gettid, err, 0);
> +
> +  int ret = INLINE_SYSCALL (tgkill, 3, pid, tid, sig);
> +
> +  __libc_signal_restore_set (&set);
> +
> +  return ret;
>  }
>  libc_hidden_def (raise)
>  weak_alias (raise, gsignal)
> 

Re: [PATCH] Refactor Linux raise implementation (BZ#15368)

[Adhemerval Zanella]

Ping x2 (I would like to push it before hard freeze).

On 29/06/2016 09:57, Adhemerval Zanella wrote:
> Ping.
> 
> On 18/06/2016 14:25, Adhemerval Zanella wrote:
>> Thanks for reviews, comment below:
>>
>> On 18/06/2016 11:09, Zack Weinberg wrote:
>>> On Fri, Jun 17, 2016 at 2:43 PM, Adhemerval Zanella
>>> <adhemerval.zanella@linaro.org> wrote:
>>>> This patch changes both the nptl and libc Linux raise implementation
>>>> to avoid the issues described in BZ#15368.
>>>
>>> It would be nice to have comments in these files which explain why it
>>> is necessary to block all signals and why the cached values are not
>>> safe to use.  The old comment that you deleted ("raise is async-safe
>>> and could be called while fork/vfork temporarily invalidated the PID")
>>> would be a good start.
>>>
>>
>> Indeed, I did not remove them in this new version below. I also extended
>> the comment a bit explaining why to block the application signals.
>>
>>>> The strategy used is
>>>> summarized in bug report first comment:
>>>>
>>>>  1. Block all signals (including internal NPTL ones);
>>>
>>> The code appears to block all signals *except* the internal NPTL ones.
>>> If I understand Rich's description of the problem correctly, that is
>>> wrong.
>>
>> Right, we need to block only user defined handler that might fork/vfork.
>> The call itself is OK, but I will change the comments describing why there
>> is no need to block GLIBC internal handlers.
>>
>>>
>>>> +static inline int
>>>> +__libc_signal_block_all (const sigset_t *set)
>>>> +{
>>>
>>> Type-safety error here: the 'set' argument is written to and should
>>> not be 'const'.  It compiles because INTERNAL_SYSCALL() erases types,
>>> but the compiler would be entitled to assume that 'set' is unchanged
>>> after the call.
>>
>> Ack.
>>
>>>
>>>> +static inline int
>>>> +__libc_signal_block_app (const sigset_t *set)
>>>
>>> Same type-safety error here.
>>
>> Ack.
>>
>>>
>>>> +  sigset_t set;
>>>> +  __libc_signal_block_app (&set);
>>>> +
>>>> +  pid_t pid = __getpid ();
>>>
>>> If I'm reading the code correctly, __getpid does *not* bypass the cache.
>>>
>>>> +  pid_t tid = INLINE_SYSCALL (gettid, 0);
>>>
>>> INTERNAL_SYSCALL() here too?  gettid() can't fail, so there's no point
>>> generating the code to potentially set errno.
>>>
>>
>> Right, I will use INTERNAL_SYSCALL.
>>
>>> zw
>>>
>>
>> I think this version addresses all the issues you raised:
>>
>> --
>>
>> 	* sysdeps/unix/sysv/linux/nptl-signals.h
>> 	(__nptl_clear_internal_signals): New function.
>> 	(__libc_signal_block_all): Likewise.
>> 	(__libc_signal_block_app): Likewise.
>> 	(__libc_signal_restore_set): Likewise.
>> 	* sysdeps/unix/sysv/linux/pt-raise.c (raise): Use Linux raise.c
>> 	implementation.
>> 	* sysdeps/unix/sysv/linux/raise.c (raise): Reimplement to not use
>> 	the cached pid/tid value in pthread structure.
>>
>> --
>>
>> diff --git a/sysdeps/unix/sysv/linux/nptl-signals.h b/sysdeps/unix/sysv/linux/nptl-signals.h
>> index 01f34c2..6525b6d 100644
>> --- a/sysdeps/unix/sysv/linux/nptl-signals.h
>> +++ b/sysdeps/unix/sysv/linux/nptl-signals.h
>> @@ -39,5 +39,42 @@ __nptl_is_internal_signal (int sig)
>>    return (sig == SIGCANCEL) || (sig == SIGTIMER) || (sig == SIGSETXID);
>>  }
>>  
>> +static inline void
>> +__nptl_clear_internal_signals (sigset_t *set)
>> +{
>> +  __sigdelset (set, SIGCANCEL);
>> +  __sigdelset (set, SIGTIMER);
>> +  __sigdelset (set, SIGSETXID);
>> +}
>> +
>> +#define SIGALL_SET \
>> +  ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })
>> +
>> +static inline int
>> +__libc_signal_block_all (sigset_t *set)
>> +{
>> +  INTERNAL_SYSCALL_DECL (err);
>> +  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &SIGALL_SET,
>> +			   set, _NSIG / 8);
>> +}
>> +
>> +static inline int
>> +__libc_signal_block_app (sigset_t *set)
>> +{
>> +  sigset_t allset = SIGALL_SET;
>> +  __nptl_clear_internal_signals (&allset);
>> +  INTERNAL_SYSCALL_DECL (err);
>> +  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &allset, set,
>> +			   _NSIG / 8);
>> +}
>> +
>> +static inline int
>> +__libc_signal_restore_set (const sigset_t *set)
>> +{
>> +  INTERNAL_SYSCALL_DECL (err);
>> +  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_SETMASK, set, NULL,
>> +			   _NSIG / 8);
>> +}
>> +
>>  /* Used to communicate with signal handler.  */
>>  extern struct xid_command *__xidcmd attribute_hidden;
>> diff --git a/sysdeps/unix/sysv/linux/pt-raise.c b/sysdeps/unix/sysv/linux/pt-raise.c
>> index 715bbe9..5f6dea1 100644
>> --- a/sysdeps/unix/sysv/linux/pt-raise.c
>> +++ b/sysdeps/unix/sysv/linux/pt-raise.c
>> @@ -1,4 +1,5 @@
>> -/* Copyright (C) 2002-2016 Free Software Foundation, Inc.
>> +/* ISO C raise function for libpthread.
>> +   Copyright (C) 2002-2016 Free Software Foundation, Inc.
>>     This file is part of the GNU C Library.
>>     Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
>>  
>> @@ -16,22 +17,4 @@
>>     License along with the GNU C Library; if not, see
>>     <http://www.gnu.org/licenses/>.  */
>>  
>> -#include <errno.h>
>> -#include <signal.h>
>> -#include <sysdep.h>
>> -#include <tls.h>
>> -
>> -
>> -int
>> -raise (int sig)
>> -{
>> -  /* raise is an async-safe function.  It could be called while the
>> -     fork function temporarily invalidated the PID field.  Adjust for
>> -     that.  */
>> -  pid_t pid = THREAD_GETMEM (THREAD_SELF, pid);
>> -  if (__glibc_unlikely (pid < 0))
>> -    pid = -pid;
>> -
>> -  return INLINE_SYSCALL (tgkill, 3, pid, THREAD_GETMEM (THREAD_SELF, tid),
>> -			 sig);
>> -}
>> +#include <sysdeps/unix/sysv/linux/raise.c>
>> diff --git a/sysdeps/unix/sysv/linux/raise.c b/sysdeps/unix/sysv/linux/raise.c
>> index 3795e6e..d386426 100644
>> --- a/sysdeps/unix/sysv/linux/raise.c
>> +++ b/sysdeps/unix/sysv/linux/raise.c
>> @@ -16,42 +16,34 @@
>>     License along with the GNU C Library; if not, see
>>     <http://www.gnu.org/licenses/>.  */
>>  
>> -#include <errno.h>
>> -#include <limits.h>
>>  #include <signal.h>
>>  #include <sysdep.h>
>> -#include <nptl/pthreadP.h>
>> -
>> +#include <errno.h>
>> +#include <sys/types.h>
>> +#include <unistd.h>
>> +#include <nptl-signals.h>
>>  
>>  int
>>  raise (int sig)
>>  {
>> -  struct pthread *pd = THREAD_SELF;
>> -  pid_t pid = THREAD_GETMEM (pd, pid);
>> -  pid_t selftid = THREAD_GETMEM (pd, tid);
>> -  if (selftid == 0)
>> -    {
>> -      /* This system call is not supposed to fail.  */
>> -#ifdef INTERNAL_SYSCALL
>> -      INTERNAL_SYSCALL_DECL (err);
>> -      selftid = INTERNAL_SYSCALL (gettid, err, 0);
>> -#else
>> -      selftid = INLINE_SYSCALL (gettid, 0);
>> -#endif
>> -      THREAD_SETMEM (pd, tid, selftid);
>> -
>> -      /* We do not set the PID field in the TID here since we might be
>> -	 called from a signal handler while the thread executes fork.  */
>> -      pid = selftid;
>> -    }
>> -  else
>> -    /* raise is an async-safe function.  It could be called while the
>> -       fork/vfork function temporarily invalidated the PID field.  Adjust for
>> -       that.  */
>> -    if (__glibc_unlikely (pid <= 0))
>> -      pid = (pid & INT_MAX) == 0 ? selftid : -pid;
>> -
>> -  return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
>> +  /* raise is an async-safe function so it could be called while the
>> +     fork/vfork function temporarily invalidated the PID field.  To avoid
>> +     relying in the cached value we block all user-defined signal handler
>> +     (which might call fork/vfork) and issues the getpid and gettid
>> +     directly.  */
>> +
>> +  sigset_t set;
>> +  __libc_signal_block_app (&set);
>> +
>> +  INTERNAL_SYSCALL_DECL (err);
>> +  pid_t pid = INTERNAL_SYSCALL (getpid, err, 0);
>> +  pid_t tid = INTERNAL_SYSCALL (gettid, err, 0);
>> +
>> +  int ret = INLINE_SYSCALL (tgkill, 3, pid, tid, sig);
>> +
>> +  __libc_signal_restore_set (&set);
>> +
>> +  return ret;
>>  }
>>  libc_hidden_def (raise)
>>  weak_alias (raise, gsignal)
>>

Re: [PATCH] Refactor Linux raise implementation (BZ#15368)

[Andreas Schwab]

Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> +  /* raise is an async-safe function so it could be called while the
> +     fork/vfork function temporarily invalidated the PID field.  To avoid
> +     relying in the cached value we block all user-defined signal handler
> +     (which might call fork/vfork) and issues the getpid and gettid
> +     directly.  */

s/issues/issue/; s/^/ syscalls/

> +  sigset_t set;
> +  __libc_signal_block_app (&set);
> +
> +  INTERNAL_SYSCALL_DECL (err);
> +  pid_t pid = INTERNAL_SYSCALL (getpid, err, 0);
> +  pid_t tid = INTERNAL_SYSCALL (gettid, err, 0);
> +
> +  int ret = INLINE_SYSCALL (tgkill, 3, pid, tid, sig);
> +
> +  __libc_signal_restore_set (&set);

What if block/unblock fail?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Refactor Linux raise implementation (BZ#15368)

[Adhemerval Zanella]

On 07/07/2016 12:30, Andreas Schwab wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> 
>> +  /* raise is an async-safe function so it could be called while the
>> +     fork/vfork function temporarily invalidated the PID field.  To avoid
>> +     relying in the cached value we block all user-defined signal handler
>> +     (which might call fork/vfork) and issues the getpid and gettid
>> +     directly.  */
> 
> s/issues/issue/; s/^/ syscalls/

Right, I will fix it.

> 
>> +  sigset_t set;
>> +  __libc_signal_block_app (&set);
>> +
>> +  INTERNAL_SYSCALL_DECL (err);
>> +  pid_t pid = INTERNAL_SYSCALL (getpid, err, 0);
>> +  pid_t tid = INTERNAL_SYSCALL (gettid, err, 0);
>> +
>> +  int ret = INLINE_SYSCALL (tgkill, 3, pid, tid, sig);
>> +
>> +  __libc_signal_restore_set (&set);
> 
> What if block/unblock fail?

My understanding checking on kernel source is 'rt_sigprocmask' may fail if:

  1. sigsetsize != sizeof (sigset_t) (EINVAL)
  2. a failure in copy_from_user/copy_to_user (EFAULT)
  3. an invalid 'how' operation (EINVAL)

The first case is already handle in glibc syscall call by using the arch 
defined _NSIG.  Second is handled by using a stack allocated mask in 'raise'
implementation.  The last one should be handled by the
__libc_signal_{un}block_{app,all} macros.

I think there is no need in this specific usage to handle failures.

Re: [PATCH] Refactor Linux raise implementation (BZ#15368)

[Andreas Schwab]

Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> I think there is no need in this specific usage to handle failures.

Could you please add a comment to that effect?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Refactor Linux raise implementation (BZ#15368)

[Adhemerval Zanella]

On 11/07/2016 04:07, Andreas Schwab wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> 
>> I think there is no need in this specific usage to handle failures.
> 
> Could you please add a comment to that effect?
> 
> Andreas.
> 

I will do it.

Re: [PATCH] Refactor Linux raise implementation (BZ#15368)

[Aurelien Jarno]

On 2016-06-17 15:43, Adhemerval Zanella wrote:
> This patch changes both the nptl and libc Linux raise implementation
> to avoid the issues described in BZ#15368.  The strategy used is
> summarized in bug report first comment:
> 
>  1. Block all signals (including internal NPTL ones);
>  2. Get pid and tid directly from syscall (not relying on cached
>     values);
>  3. Call tgkill;
>  4. Restore old signal mask.

This new implementation introduces a behaviour change when a process is
run under ptrace:
  1) The process call raise(SIGSTOP)
  2) The parent process run ptrace (PTRACE_CONT, pid, NULL, SOME_SIGNAL)
  3) The process runs some code generating a ptrace event

With the old implementation, the ptrace event captured after the process
is restarted is the one from 3). With the new implementation, given the
signals are blocked, they are only delivered when raise unblocks them.
This generates an additional ptrace event for the delivery of
SOME_SIGNAL before 3).

For reference this breaks the libnih testsuite. I believe that it is a 
corner case and that the testsuite has too precise expectations. Still
I think it is worth mentioning the behavior change here in case someone
ends up debugging the same issue.

Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net