commit a8ac8c4725ddb1119764126a8674a04c9dd5aea8

Author: Florian Weimer <fweimer@redhat.com>

Date:   Mon Sep 13 11:06:08 2021 +0200

    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>

    (cherry picked from commit 526c3cf11ee9367344b6b15d669e4c3cb461a2be)

diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c

index cfe37a3443b69454..50065bc9bd8a28e5 100644

--- a/nptl/allocatestack.c

+++ b/nptl/allocatestack.c

@@ -32,6 +32,7 @@

 #include <futex-internal.h>

 #include <kernel-features.h>

 #include <nptl-stack.h>

+#include <libc-lock.h>

 /* Default alignment of stack.  */

 #ifndef STACK_ALIGN

@@ -127,6 +128,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 c85778d44941a42f..4de84138fb960fa4 100644

--- a/nptl/descr.h

+++ b/nptl/descr.h

@@ -396,6 +396,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 d8ec299cb1661e82..33b426fc682300dc 100644

--- a/nptl/pthread_create.c

+++ b/nptl/pthread_create.c

@@ -37,6 +37,7 @@

 #include <sys/single_threaded.h>

 #include <version.h>

 #include <clone_internal.h>

+#include <futex-internal.h>

 #include <shlib-compat.h>

@@ -485,6 +486,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 5d4c86f9205a6fb5..fb7862eff787a94f 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

diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile

index dedfa0d290da4949..48dba717a1cdc20a 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-pthread_cancel-select-loop.c b/sysdeps/pthread/tst-pthread_cancel-select-loop.c

new file mode 100644

index 0000000000000000..a62087589cee24b5

--- /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 0000000000000000..f803e94f1195f204

--- /dev/null

+++ b/sysdeps/pthread/tst-pthread_kill-exiting.c

@@ -0,0 +1,123 @@

+/* 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;

+

+/* Set by the main thread to true after timeout has been set to

+   true.  */

+static bool exiting;

+

+static void *

+sender_thread_function (void *unused)

+{

+  while (true)

+    {

+      /* Wait until target_thread has been initialized.  The target

+         thread and main thread participate in this barrier.  */

+      xpthread_barrier_wait (&barrier_1);

+

+      if (exiting)

+        break;

+

+      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);

+    }

+

+  exiting = true;

+

+  /* Signal the sending threads to exit.  */

+  xpthread_create (NULL, target_thread_function, NULL);

+  xpthread_barrier_wait (&barrier_1);

+

+  for (int i = 0; i < array_length (threads); ++i)

+    xpthread_join (threads[i]);

+  xpthread_join (thr_timeout);

+

+  return 0;

+}

+

+#include <support/test-driver.c>