[v2,08/19] nptl: pthread_kill, pthread_cancel should fail after exit (bug 19193)

Message ID 20210823195047.543237-9-adhemerval.zanella@linaro.org
State Superseded
Headers
Series Fix various NPTL synchronization issues |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Adhemerval Zanella Aug. 23, 2021, 7:50 p.m. UTC
  From: Florian Weimer <fweimer@redhat.com>

This closes one remaining race condition related to bug 12889: if
the thread already exited on the kernel side, returning ESRCH
is not correct because that error is reserved for the thread IDs
(pthread_t values) whose lifetime has ended.  In case of a
kernel-side exit and a valid thread ID, no signal needs to be sent
and cancellation does not have an effect, so just return 0.

sysdeps/pthread/tst-kill4.c triggers undefined behavior and is
removed with this commit.

Checked on x86_64-linux-gnu and i686-linux-gnu.
---
 nptl/pthread_cancel.c                       |  6 ++-
 nptl/pthread_kill.c                         |  7 +++-
 sysdeps/pthread/Makefile                    |  5 ++-
 sysdeps/pthread/tst-pthread_cancel-exited.c | 45 ++++++++++++++++++++
 sysdeps/pthread/tst-pthread_kill-exited.c   | 46 +++++++++++++++++++++
 5 files changed, 105 insertions(+), 4 deletions(-)
 create mode 100644 sysdeps/pthread/tst-pthread_cancel-exited.c
 create mode 100644 sysdeps/pthread/tst-pthread_kill-exited.c
  

Comments

Florian Weimer Aug. 26, 2021, 10:03 a.m. UTC | #1
* Adhemerval Zanella:

> From: Florian Weimer <fweimer@redhat.com>
>
> This closes one remaining race condition related to bug 12889: if
> the thread already exited on the kernel side, returning ESRCH
> is not correct because that error is reserved for the thread IDs
> (pthread_t values) whose lifetime has ended.  In case of a
> kernel-side exit and a valid thread ID, no signal needs to be sent
> and cancellation does not have an effect, so just return 0.
>
> sysdeps/pthread/tst-kill4.c triggers undefined behavior and is
> removed with this commit.

Wrong commit subject: “should [not] fail”

Thanks,
Florian
  
Adhemerval Zanella Aug. 26, 2021, 4:49 p.m. UTC | #2
On 26/08/2021 07:03, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> From: Florian Weimer <fweimer@redhat.com>
>>
>> This closes one remaining race condition related to bug 12889: if
>> the thread already exited on the kernel side, returning ESRCH
>> is not correct because that error is reserved for the thread IDs
>> (pthread_t values) whose lifetime has ended.  In case of a
>> kernel-side exit and a valid thread ID, no signal needs to be sent
>> and cancellation does not have an effect, so just return 0.
>>
>> sysdeps/pthread/tst-kill4.c triggers undefined behavior and is
>> removed with this commit.
> 
> Wrong commit subject: “should [not] fail”

Ack.
  

Patch

diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
index aed6c1ea47..6f6c989dc0 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -62,8 +62,10 @@  __pthread_cancel (pthread_t th)
   /* Make sure the descriptor is valid.  */
   int state = atomic_load_acquire (&pd->joinstate);
   if (state == THREAD_STATE_EXITED || state == THREAD_STATE_EXITING)
-    /* Not a valid thread handle.  */
-    return ESRCH;
+    /* The thread has already either exited on the kernel side or started the
+       exit phase.  In eitehr case its outcome (regular exit, other
+       cancelation) has already been determined.  */
+    return 0;
 
   static int init_sigcancel = 0;
   if (atomic_load_relaxed (&init_sigcancel) == 0)
diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c
index f79a2b26fc..5d4c86f920 100644
--- a/nptl/pthread_kill.c
+++ b/nptl/pthread_kill.c
@@ -46,7 +46,12 @@  __pthread_kill_internal (pthread_t threadid, int signo)
 	    ? INTERNAL_SYSCALL_ERRNO (val) : 0);
     }
   else
-    val = ESRCH;
+    /* 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;
 
   return val;
 }
diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
index 42f9fc5072..dedfa0d290 100644
--- a/sysdeps/pthread/Makefile
+++ b/sysdeps/pthread/Makefile
@@ -89,7 +89,7 @@  tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \
 	 tst-join8 tst-join9 tst-join10 tst-join11 tst-join12 tst-join13 \
 	 tst-join14 tst-join15 \
 	 tst-key1 tst-key2 tst-key3 tst-key4 \
-	 tst-kill1 tst-kill2 tst-kill3 tst-kill4 tst-kill5 tst-kill6 \
+	 tst-kill1 tst-kill2 tst-kill3 tst-kill5 tst-kill6 \
 	 tst-locale1 tst-locale2 \
 	 tst-memstream \
 	 tst-mutex-errorcheck tst-mutex1 tst-mutex2 tst-mutex3 tst-mutex4 \
@@ -118,6 +118,9 @@  tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \
 	 tst-unload \
 	 tst-unwind-thread \
 	 tst-pt-vfork1 tst-pt-vfork2 tst-vfork1x tst-vfork2x \
+	 tst-pthread_cancel-exited \
+	 tst-pthread_kill-exited \
+	 # tests
 
 tests-time64 := \
   tst-abstime-time64 \
diff --git a/sysdeps/pthread/tst-pthread_cancel-exited.c b/sysdeps/pthread/tst-pthread_cancel-exited.c
new file mode 100644
index 0000000000..811c9bee07
--- /dev/null
+++ b/sysdeps/pthread/tst-pthread_cancel-exited.c
@@ -0,0 +1,45 @@ 
+/* Test that pthread_kill succeeds for an exited thread.
+   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 returns 0 (and not ESRCH) for
+   a thread that has exited on the kernel side.  */
+
+#include <stddef.h>
+#include <support/support.h>
+#include <support/xthread.h>
+
+static void *
+noop_thread (void *closure)
+{
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  pthread_t thr = xpthread_create (NULL, noop_thread, NULL);
+
+  support_wait_for_thread_exit ();
+
+  xpthread_cancel (thr);
+  xpthread_join (thr);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/sysdeps/pthread/tst-pthread_kill-exited.c b/sysdeps/pthread/tst-pthread_kill-exited.c
new file mode 100644
index 0000000000..7575fb6d58
--- /dev/null
+++ b/sysdeps/pthread/tst-pthread_kill-exited.c
@@ -0,0 +1,46 @@ 
+/* Test that pthread_kill succeeds for an exited thread.
+   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 returns 0 (and not ESRCH) for
+   a thread that has exited on the kernel side.  */
+
+#include <signal.h>
+#include <stddef.h>
+#include <support/support.h>
+#include <support/xthread.h>
+
+static void *
+noop_thread (void *closure)
+{
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  pthread_t thr = xpthread_create (NULL, noop_thread, NULL);
+
+  support_wait_for_thread_exit ();
+
+  xpthread_kill (thr, SIGUSR1);
+  xpthread_join (thr);
+
+  return 0;
+}
+
+#include <support/test-driver.c>