nptl: Fix race between pthread_kill and thread exit (bug 12889)

A new thread exit lock and flag are introduced.  They are used to
detect that the thread is about to exit or has exited in
__pthread_kill_internal, and the signal is not sent in this case.

The test sysdeps/pthread/tst-pthread_cancel-select-loop.c is derived
from a downstream test originally written by Marek Polacek.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

4 files changed, 63 insertions, 25 deletions

diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index 0356993c26..fa8100719f 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -31,6 +31,7 @@
 #include <futex-internal.h>
 #include <kernel-features.h>
 #include <nptl-stack.h>
+#include <libc-lock.h>
 
 /* Default alignment of stack.  */
 #ifndef STACK_ALIGN
@@ -126,6 +127,8 @@ get_cached_stack (size_t *sizep, void **memp)
   /* No pending event.  */
   result->nextevent = NULL;
 
+  result->exiting = false;
+  __libc_lock_init (result->exit_lock);
   result->tls_state = (struct tls_internal_t) { 0 };
 
   /* Clear the DTV.  */
diff --git a/nptl/descr.h b/nptl/descr.h
index e1c8831f09..41ee56feb2 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -395,6 +395,12 @@ struct pthread
      PTHREAD_CANCEL_ASYNCHRONOUS).  */
   unsigned char canceltype;
 
+  /* Used in __pthread_kill_internal to detected a thread that has
+     exited or is about to exit.  exit_lock must only be acquired
+     after blocking signals.  */
+  bool exiting;
+  int exit_lock; /* A low-level lock (for use with __libc_lock_init etc).  */
+
   /* Used on strsignal.  */
   struct tls_internal_t tls_state;
 
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index 7607f36e26..a559f86cc2 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -36,6 +36,7 @@
 #include <sys/single_threaded.h>
 #include <version.h>
 #include <clone_internal.h>
+#include <futex-internal.h>
 
 #include <shlib-compat.h>
 
@@ -484,6 +485,19 @@ start_thread (void *arg)
     /* This was the last thread.  */
     exit (0);
 
+  /* This prevents sending a signal from this thread to itself during
+     its final stages.  This must come after the exit call above
+     because atexit handlers must not run with signals blocked.  */
+  __libc_signal_block_all (NULL);
+
+  /* Tell __pthread_kill_internal that this thread is about to exit.
+     If there is a __pthread_kill_internal in progress, this delays
+     the thread exit until the signal has been queued by the kernel
+     (so that the TID used to send it remains valid).  */
+  __libc_lock_lock (pd->exit_lock);
+  pd->exiting = true;
+  __libc_lock_unlock (pd->exit_lock);
+
 #ifndef __ASSUME_SET_ROBUST_LIST
   /* If this thread has any robust mutexes locked, handle them now.  */
 # if __PTHREAD_MUTEX_HAVE_PREV
diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c
index 5d4c86f920..fb7862eff7 100644
--- a/nptl/pthread_kill.c
+++ b/nptl/pthread_kill.c
@@ -16,6 +16,7 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
+#include <libc-lock.h>
 #include <unistd.h>
 #include <pthreadP.h>
 #include <shlib-compat.h>
@@ -23,37 +24,51 @@
 int
 __pthread_kill_internal (pthread_t threadid, int signo)
 {
-  pid_t tid;
   struct pthread *pd = (struct pthread *) threadid;
-
   if (pd == THREAD_SELF)
-    /* It is a special case to handle raise() implementation after a vfork
-       call (which does not update the PD tid field).  */
-    tid = INLINE_SYSCALL_CALL (gettid);
-  else
-    /* Force load of pd->tid into local variable or register.  Otherwise
-       if a thread exits between ESRCH test and tgkill, we might return
-       EINVAL, because pd->tid would be cleared by the kernel.  */
-    tid = atomic_forced_read (pd->tid);
-
-  int val;
-  if (__glibc_likely (tid > 0))
     {
-      pid_t pid = __getpid ();
-
-      val = INTERNAL_SYSCALL_CALL (tgkill, pid, tid, signo);
-      val = (INTERNAL_SYSCALL_ERROR_P (val)
-	    ? INTERNAL_SYSCALL_ERRNO (val) : 0);
+      /* Use the actual TID from the kernel, so that it refers to the
+         current thread even if called after vfork.  There is no
+         signal blocking in this case, so that the signal is delivered
+         immediately, before __pthread_kill_internal returns: a signal
+         sent to the thread itself needs to be delivered
+         synchronously.  (It is unclear if Linux guarantees the
+         delivery of all pending signals after unblocking in the code
+         below.  POSIX only guarantees delivery of a single signal,
+         which may not be the right one.)  */
+      pid_t tid = INTERNAL_SYSCALL_CALL (gettid);
+      int ret = INTERNAL_SYSCALL_CALL (kill, tid, signo);
+      return INTERNAL_SYSCALL_ERROR_P (ret) ? INTERNAL_SYSCALL_ERRNO (ret) : 0;
     }
+
+  /* Block all signals, as required by pd->exit_lock.  */
+  sigset_t old_mask;
+  __libc_signal_block_all (&old_mask);
+  __libc_lock_lock (pd->exit_lock);
+
+  int ret;
+  if (pd->exiting)
+    /* The thread is about to exit (or has exited).  Sending the
+       signal is either not observable (the target thread has already
+       blocked signals at this point), or it will fail, or it might be
+       delivered to a new, unrelated thread that has reused the TID.
+       So do not actually send the signal.  Do not report an error
+       because the threadid argument is still valid (the thread ID
+       lifetime has not ended), and ESRCH (for example) would be
+       misleading.  */
+    ret = 0;
   else
-    /* The kernel reports that the thread has exited.  POSIX specifies
-       the ESRCH error only for the case when the lifetime of a thread
-       ID has ended, but calling pthread_kill on such a thread ID is
-       undefined in glibc.  Therefore, do not treat kernel thread exit
-       as an error.  */
-    val = 0;
+    {
+      /* Using tgkill is a safety measure.  pd->exit_lock ensures that
+	 the target thread cannot exit.  */
+      ret = INTERNAL_SYSCALL_CALL (tgkill, __getpid (), pd->tid, signo);
+      ret = INTERNAL_SYSCALL_ERROR_P (ret) ? INTERNAL_SYSCALL_ERRNO (ret) : 0;
+    }
+
+  __libc_lock_unlock (pd->exit_lock);
+  __libc_signal_restore_set (&old_mask);
 
-  return val;
+  return ret;
 }
 
 int