[v3,2/2] nptl: Fix race between pthread_kill and thread exit (bug 12889)

Message ID 360490024b83370b3f18258694b00165827d5a05.1630931178.git.fweimer@redhat.com
State New
Headers show
Series
  • Fix pthread_cancel, pthread_kill race (bug 12889)
Related show

Commit Message

Noah Goldstein via Libc-alpha Sept. 6, 2021, 12:34 p.m.
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.
---
 nptl/allocatestack.c                          |   3 +
 nptl/descr.h                                  |   6 +
 nptl/pthread_create.c                         |  14 ++
 nptl/pthread_kill.c                           |  65 +++++----
 sysdeps/pthread/Makefile                      |   2 +
 .../pthread/tst-pthread_cancel-select-loop.c  |  87 +++++++++++++
 sysdeps/pthread/tst-pthread_kill-exiting.c    | 123 ++++++++++++++++++
 7 files changed, 275 insertions(+), 25 deletions(-)
 create mode 100644 sysdeps/pthread/tst-pthread_cancel-select-loop.c
 create mode 100644 sysdeps/pthread/tst-pthread_kill-exiting.c

-- 
2.31.1

Comments

Noah Goldstein via Libc-alpha Sept. 7, 2021, 1:21 p.m. | #1
On 06/09/2021 09:34, Florian Weimer via Libc-alpha wrote:
> 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.


LGTM, with just a suggestion to use C11 atomics instead of atomic
builtins on the tests.

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


> ---

>  nptl/allocatestack.c                          |   3 +

>  nptl/descr.h                                  |   6 +

>  nptl/pthread_create.c                         |  14 ++

>  nptl/pthread_kill.c                           |  65 +++++----

>  sysdeps/pthread/Makefile                      |   2 +

>  .../pthread/tst-pthread_cancel-select-loop.c  |  87 +++++++++++++

>  sysdeps/pthread/tst-pthread_kill-exiting.c    | 123 ++++++++++++++++++

>  7 files changed, 275 insertions(+), 25 deletions(-)

>  create mode 100644 sysdeps/pthread/tst-pthread_cancel-select-loop.c

>  create mode 100644 sysdeps/pthread/tst-pthread_kill-exiting.c

> 

> 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.  */


Ok.

> 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


Ok.

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

>      }


Ok.

> +

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


Ok.

> 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 := \


Ok.

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


Why not use C11 atomics here? Same for other atomic builts usage.

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


Ok.

> diff --git a/sysdeps/pthread/tst-pthread_kill-exiting.c b/sysdeps/pthread/tst-pthread_kill-exiting.c

> new file mode 100644

> index 0000000000..f803e94f11

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


Why not use C11 atomics here? Same for other atomic builts usage.

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

> 


Ok.
Noah Goldstein via Libc-alpha Sept. 10, 2021, 3:19 p.m. | #2
* Adhemerval Zanella:

> On 06/09/2021 09:34, Florian Weimer via Libc-alpha wrote:

>> 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.

>

> LGTM, with just a suggestion to use C11 atomics instead of atomic

> builtins on the tests.


I prefer the __atomic built-ins because we have free documentation for
them.  Still okay to use them?

Thanks,
Florian

Patch

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
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-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..f803e94f11
--- /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>