[v3,1/2] nptl: pthread_kill, pthread_cancel should not fail after exit (bug 19193)
Checks
| Context |
Check |
Description |
| dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
Commit Message
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.
---
nptl/pthread_cancel.c | 9 ++-
nptl/pthread_kill.c | 7 +-
sysdeps/pthread/Makefile | 5 +-
sysdeps/pthread/tst-kill4.c | 89 ---------------------
sysdeps/pthread/tst-pthread_cancel-exited.c | 45 +++++++++++
sysdeps/pthread/tst-pthread_kill-exited.c | 46 +++++++++++
6 files changed, 106 insertions(+), 95 deletions(-)
delete mode 100644 sysdeps/pthread/tst-kill4.c
create mode 100644 sysdeps/pthread/tst-pthread_cancel-exited.c
create mode 100644 sysdeps/pthread/tst-pthread_kill-exited.c
Comments
On 06/09/2021 09:33, Florian Weimer via Libc-alpha wrote:
> 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.
LGTM, thanks.
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> ---
> nptl/pthread_cancel.c | 9 ++-
> nptl/pthread_kill.c | 7 +-
> sysdeps/pthread/Makefile | 5 +-
> sysdeps/pthread/tst-kill4.c | 89 ---------------------
> sysdeps/pthread/tst-pthread_cancel-exited.c | 45 +++++++++++
> sysdeps/pthread/tst-pthread_kill-exited.c | 46 +++++++++++
> 6 files changed, 106 insertions(+), 95 deletions(-)
> delete mode 100644 sysdeps/pthread/tst-kill4.c
> create mode 100644 sysdeps/pthread/tst-pthread_cancel-exited.c
> create mode 100644 sysdeps/pthread/tst-pthread_kill-exited.c
>
> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
> index 2bb523c0ec..a8aa3b3d15 100644
> --- a/nptl/pthread_cancel.c
> +++ b/nptl/pthread_cancel.c
> @@ -61,10 +61,11 @@ __pthread_cancel (pthread_t th)
> {
> volatile struct pthread *pd = (volatile struct pthread *) th;
>
> - /* Make sure the descriptor is valid. */
> - if (INVALID_TD_P (pd))
> - /* Not a valid thread handle. */
> - return ESRCH;
> + if (pd->tid == 0)
> + /* The thread has already exited on the kernel side. Its outcome
> + (regular exit, other cancelation) has already been
> + determined. */
> + return 0;
>
> static int init_sigcancel = 0;
> if (atomic_load_relaxed (&init_sigcancel) == 0)
Ok.
> 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;
> }
Ok.
> 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 \
Ok.
> diff --git a/sysdeps/pthread/tst-kill4.c b/sysdeps/pthread/tst-kill4.c
> deleted file mode 100644
> index 222ceb724c..0000000000
> --- a/sysdeps/pthread/tst-kill4.c
> +++ /dev/null
> @@ -1,89 +0,0 @@
> -/* Copyright (C) 2003-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/>. */
> -
> -#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"
Ok.
> 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>
Ok.
> 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>
>
Ok.
@@ -61,10 +61,11 @@ __pthread_cancel (pthread_t th)
{
volatile struct pthread *pd = (volatile struct pthread *) th;
- /* Make sure the descriptor is valid. */
- if (INVALID_TD_P (pd))
- /* Not a valid thread handle. */
- return ESRCH;
+ if (pd->tid == 0)
+ /* The thread has already exited on the kernel side. Its outcome
+ (regular exit, other cancelation) has already been
+ determined. */
+ return 0;
static int init_sigcancel = 0;
if (atomic_load_relaxed (&init_sigcancel) == 0)
@@ -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;
}
@@ -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 \
deleted file mode 100644
@@ -1,89 +0,0 @@
-/* Copyright (C) 2003-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/>. */
-
-#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"
new file mode 100644
@@ -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>
new file mode 100644
@@ -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>