[glibc/azanella/pthread-multiple-fixes] nptl: Fix race between pthread_kill and thread exit (bug 12889)

[Adhemerval Zanella <azanella@sourceware.org>]

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=bb94df8eaba1af636bd6af7ec9142bc3e0e25868

commit bb94df8eaba1af636bd6af7ec9142bc3e0e25868
Author: Florian Weimer <fweimer@redhat.com>
Date:   Fri Aug 20 13:45:07 2021 -0300

    nptl: Fix race between pthread_kill and thread exit (bug 12889)
    
    A new lock is used to synchronize between pthread_kill and thread exit.
    
    Checked on x86_64-linux-gnu.
    
    Co-authored-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

Diff:
---
 nptl/allocatestack.c                             |   1 +
 nptl/descr.h                                     |   3 +
 nptl/pthread_create.c                            |   7 ++
 nptl/pthread_kill.c                              |   9 ++
 sysdeps/pthread/Makefile                         |   2 +
 sysdeps/pthread/tst-kill4.c                      |  90 -------------------
 sysdeps/pthread/tst-pthread_cancel-select-loop.c |  87 ++++++++++++++++++
 sysdeps/pthread/tst-pthread_kill-exiting.c       | 110 +++++++++++++++++++++++
 8 files changed, 219 insertions(+), 90 deletions(-)

diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index a2a1fa7b81..84e8972eab 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -124,6 +124,7 @@ get_cached_stack (size_t *sizep, void **memp)
   result->canceltype = PTHREAD_CANCEL_DEFERRED;
   result->cleanup = NULL;
   result->setup_failed = 0;
+  result->killlock = 0;
 
   /* No pending event.  */
   result->nextevent = NULL;
diff --git a/nptl/descr.h b/nptl/descr.h
index 1bfa2b9b52..d69e2a3df4 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -327,6 +327,9 @@ struct pthread
   /* Lock to synchronize access to the descriptor.  */
   int lock;
 
+  /* Lock to synchronize pthread_kill and thread exit.  */
+  int killlock;
+
   /* Indicate whether thread is supposed to change XID.  */
   int setxid_flag;
   /* Lock for synchronizing setxid calls.  */
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index b354f62995..5b51f54190 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -531,6 +531,12 @@ 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);
+  lll_lock (pd->killlock, LLL_PRIVATE);
+
   if (!pd->user_stack)
     advise_stack_range (pd->stackblock, pd->stackblock_size, (uintptr_t) pd,
 			pd->guardsize);
@@ -555,6 +561,7 @@ start_thread (void *arg)
     __nptl_free_tcb (pd);
 
   pd->tid = 0;
+  lll_unlock (pd->killlock, LLL_PRIVATE);
 
 out:
   /* We cannot call '_exit' here.  '_exit' will terminate the process.
diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c
index f935b1fb3e..23f3f03b6e 100644
--- a/nptl/pthread_kill.c
+++ b/nptl/pthread_kill.c
@@ -26,6 +26,12 @@ __pthread_kill_internal (pthread_t threadid, int signo)
   pid_t tid;
   struct pthread *pd = (struct pthread *) threadid;
 
+  /* Block all signal, since the lock is recursive and used on pthread_cnacel
+     (which should be async-signal-safe).  */
+  sigset_t oldmask;
+  __libc_signal_block_all (&oldmask);
+  lll_lock (pd->killlock, LLL_PRIVATE);
+
   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).  */
@@ -48,6 +54,9 @@ __pthread_kill_internal (pthread_t threadid, int signo)
   else
     val = 0;
 
+  lll_unlock (pd->killlock, LLL_PRIVATE);
+  __libc_signal_restore_set (&oldmask);
+
   return val;
 }
 
diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
index dedfa0d290..48dba717a1 100644
--- a/sysdeps/pthread/Makefile
+++ b/sysdeps/pthread/Makefile
@@ -119,7 +119,9 @@ tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \
 	 tst-unwind-thread \
 	 tst-pt-vfork1 tst-pt-vfork2 tst-vfork1x tst-vfork2x \
 	 tst-pthread_cancel-exited \
+	 tst-pthread_cancel-select-loop \
 	 tst-pthread_kill-exited \
+	 tst-pthread_kill-exiting \
 	 # tests
 
 tests-time64 := \
diff --git a/sysdeps/pthread/tst-kill4.c b/sysdeps/pthread/tst-kill4.c
deleted file mode 100644
index 9563939792..0000000000
--- a/sysdeps/pthread/tst-kill4.c
+++ /dev/null
@@ -1,90 +0,0 @@
-/* Copyright (C) 2003-2021 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Contributed by Ulrich Drepper <drepper@redhat.com>, 2003.
-
-   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
-   <https://www.gnu.org/licenses/>.  */
-
-#include <errno.h>
-#include <pthread.h>
-#include <signal.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <unistd.h>
-
-
-static void *
-tf (void *a)
-{
-  return NULL;
-}
-
-
-int
-do_test (void)
-{
-  pthread_attr_t at;
-  if (pthread_attr_init (&at) != 0)
-    {
-      puts ("attr_create failed");
-      exit (1);
-    }
-
-  /* Limit thread stack size, because if it is too large, pthread_join
-     will free it immediately rather than put it into stack cache.  */
-  if (pthread_attr_setstacksize (&at, 2 * 1024 * 1024) != 0)
-    {
-      puts ("setstacksize failed");
-      exit (1);
-    }
-
-  pthread_t th;
-  if (pthread_create (&th, &at, tf, NULL) != 0)
-    {
-      puts ("create failed");
-      exit (1);
-    }
-
-  pthread_attr_destroy (&at);
-
-  if (pthread_join (th, NULL) != 0)
-    {
-      puts ("join failed");
-      exit (1);
-    }
-
-  /* The following only works because we assume here something about
-     the implementation.  Namely, that the memory allocated for the
-     thread descriptor is not going away, that the TID field is
-     cleared and therefore the signal is sent to process 0, and that
-     we can savely assume there is no other process with this ID at
-     that time.  */
-  int e = pthread_kill (th, 0);
-  if (e == 0)
-    {
-      puts ("pthread_kill succeeded");
-      exit (1);
-    }
-  if (e != ESRCH)
-    {
-      puts ("pthread_kill didn't return ESRCH");
-      exit (1);
-    }
-
-  return 0;
-}
-
-
-#define TEST_FUNCTION do_test ()
-#include "../test-skeleton.c"
diff --git a/sysdeps/pthread/tst-pthread_cancel-select-loop.c b/sysdeps/pthread/tst-pthread_cancel-select-loop.c
new file mode 100644
index 0000000000..a62087589c
--- /dev/null
+++ b/sysdeps/pthread/tst-pthread_cancel-select-loop.c
@@ -0,0 +1,87 @@
+/* Test that pthread_cancel succeeds during thread exit.
+   Copyright (C) 2021 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
+   <https://www.gnu.org/licenses/>.  */
+
+/* This test tries to trigger an internal race condition in
+   pthread_cancel, where the cancellation signal is sent after the
+   thread has begun the cancellation process.  This can result in a
+   spurious ESRCH error.  For the original bug 12889, the window is
+   quite small, so the bug was not reproduced in every run.  */
+
+#include <stdbool.h>
+#include <stddef.h>
+#include <support/check.h>
+#include <support/xthread.h>
+#include <support/xunistd.h>
+#include <sys/select.h>
+#include <unistd.h>
+
+/* Set to true by timeout_thread_function when the test should
+   terminate.  */
+static bool timeout;
+
+static void *
+timeout_thread_function (void *unused)
+{
+  usleep (5 * 1000 * 1000);
+  __atomic_store_n (&timeout, true, __ATOMIC_RELAXED);
+  return NULL;
+}
+
+/* Used for blocking the select function below.  */
+static int pipe_fds[2];
+
+static void *
+canceled_thread_function (void *unused)
+{
+  while (true)
+    {
+      fd_set rfs;
+      fd_set wfs;
+      fd_set efs;
+      FD_ZERO (&rfs);
+      FD_ZERO (&wfs);
+      FD_ZERO (&efs);
+      FD_SET (pipe_fds[0], &rfs);
+
+      /* If the cancellation request is recognized early, the thread
+         begins exiting while the cancellation signal arrives.  */
+      select (FD_SETSIZE, &rfs, &wfs, &efs, NULL);
+    }
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  xpipe (pipe_fds);
+  pthread_t thr_timeout = xpthread_create (NULL, timeout_thread_function, NULL);
+
+  while (!__atomic_load_n (&timeout, __ATOMIC_RELAXED))
+    {
+      pthread_t thr = xpthread_create (NULL, canceled_thread_function, NULL);
+      xpthread_cancel (thr);
+      TEST_VERIFY (xpthread_join (thr) == PTHREAD_CANCELED);
+    }
+
+  xpthread_join (thr_timeout);
+  xclose (pipe_fds[0]);
+  xclose (pipe_fds[1]);
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/sysdeps/pthread/tst-pthread_kill-exiting.c b/sysdeps/pthread/tst-pthread_kill-exiting.c
new file mode 100644
index 0000000000..15550d2310
--- /dev/null
+++ b/sysdeps/pthread/tst-pthread_kill-exiting.c
@@ -0,0 +1,110 @@
+/* Test that pthread_kill succeeds during thread exit.
+   Copyright (C) 2021 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
+   <https://www.gnu.org/licenses/>.  */
+
+/* This test verifies that pthread_kill for a thread that is exiting
+   succeeds (with or without actually delivering the signal).  */
+
+#include <array_length.h>
+#include <stdbool.h>
+#include <stddef.h>
+#include <support/xsignal.h>
+#include <support/xthread.h>
+#include <unistd.h>
+
+/* Set to true by timeout_thread_function when the test should
+   terminate.  */
+static bool timeout;
+
+static void *
+timeout_thread_function (void *unused)
+{
+  usleep (1000 * 1000);
+  __atomic_store_n (&timeout, true, __ATOMIC_RELAXED);
+  return NULL;
+}
+
+/* Used to synchronize the sending threads with the target thread and
+   main thread.  */
+static pthread_barrier_t barrier_1;
+static pthread_barrier_t barrier_2;
+
+/* The target thread to which signals are to be sent.  */
+static pthread_t target_thread;
+
+static void *
+sender_thread_function (void *unused)
+{
+  while (!__atomic_load_n (&timeout, __ATOMIC_RELAXED))
+    {
+      /* Wait until target_thread has been initialized.  The target
+         thread and main thread participate in this barrier.  */
+      xpthread_barrier_wait (&barrier_1);
+
+      xpthread_kill (target_thread, SIGUSR1);
+
+      /* Communicate that the signal has been sent.  The main thread
+         participates in this barrier.  */
+      xpthread_barrier_wait (&barrier_2);
+    }
+  return NULL;
+}
+
+static void *
+target_thread_function (void *unused)
+{
+  target_thread = pthread_self ();
+  xpthread_barrier_wait (&barrier_1);
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  xsignal (SIGUSR1, SIG_IGN);
+
+  pthread_t thr_timeout = xpthread_create (NULL, timeout_thread_function, NULL);
+
+  pthread_t threads[4];
+  xpthread_barrier_init (&barrier_1, NULL, array_length (threads) + 2);
+  xpthread_barrier_init (&barrier_2, NULL, array_length (threads) + 1);
+
+  for (int i = 0; i < array_length (threads); ++i)
+    threads[i] = xpthread_create (NULL, sender_thread_function, NULL);
+
+  while (!__atomic_load_n (&timeout, __ATOMIC_RELAXED))
+    {
+      xpthread_create (NULL, target_thread_function, NULL);
+
+      /* Wait for the target thread to be set up and signal sending to
+         start.  */
+      xpthread_barrier_wait (&barrier_1);
+
+      /* Wait for signal sending to complete.  */
+      xpthread_barrier_wait (&barrier_2);
+
+      xpthread_join (target_thread);
+    }
+
+  for (int i = 0; i < array_length (threads); ++i)
+    xpthread_join (threads[i]);
+  xpthread_join (thr_timeout);
+
+  return 0;
+}
+
+#include <support/test-driver.c>