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

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

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)