* [PATCH v3 0/2] Fix pthread_cancel, pthread_kill race (bug 12889)
@ 2021-09-06 12:33 Florian Weimer via Libc-alpha
2021-09-06 12:33 ` [PATCH v3 1/2] nptl: pthread_kill, pthread_cancel should not fail after exit (bug 19193) Florian Weimer via Libc-alpha
2021-09-06 12:34 ` [PATCH v3 2/2] nptl: Fix race between pthread_kill and thread exit (bug 12889) Florian Weimer via Libc-alpha
0 siblings, 2 replies; 6+ messages in thread
From: Florian Weimer via Libc-alpha @ 2021-09-06 12:33 UTC (permalink / raw)
To: libc-alpha
This is a repost of the previous series, with an internal (futex-based)
lock and separate flag in struct pthread. __pthread_call_with_tid is
gone (it's inlined into __pthread_kill_internal).
This is the version I propose to backport to glibc 2.34. I think it
should go into the main branch as well, unless Adhemerval's larger
series is committed first.
I had to fix a defect in the tst-pthread_kill-exiting. The sending
threads were not signaled in all cases, leading to a deadlock.
Thanks,
Florian
Florian Weimer (2):
nptl: pthread_kill, pthread_cancel should not fail after exit (bug
19193)
nptl: Fix race between pthread_kill and thread exit (bug 12889)
nptl/allocatestack.c | 3 +
nptl/descr.h | 6 +
nptl/pthread_cancel.c | 9 +-
nptl/pthread_create.c | 14 ++
nptl/pthread_kill.c | 60 ++++++---
sysdeps/pthread/Makefile | 7 +-
sysdeps/pthread/tst-kill4.c | 89 -------------
sysdeps/pthread/tst-pthread_cancel-exited.c | 45 +++++++
.../pthread/tst-pthread_cancel-select-loop.c | 87 +++++++++++++
sysdeps/pthread/tst-pthread_kill-exited.c | 46 +++++++
sysdeps/pthread/tst-pthread_kill-exiting.c | 123 ++++++++++++++++++
11 files changed, 375 insertions(+), 114 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_cancel-select-loop.c
create mode 100644 sysdeps/pthread/tst-pthread_kill-exited.c
create mode 100644 sysdeps/pthread/tst-pthread_kill-exiting.c
--
2.31.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/2] nptl: pthread_kill, pthread_cancel should not fail after exit (bug 19193)
2021-09-06 12:33 [PATCH v3 0/2] Fix pthread_cancel, pthread_kill race (bug 12889) Florian Weimer via Libc-alpha
@ 2021-09-06 12:33 ` Florian Weimer via Libc-alpha
2021-09-07 12:42 ` Adhemerval Zanella via Libc-alpha
2021-09-06 12:34 ` [PATCH v3 2/2] nptl: Fix race between pthread_kill and thread exit (bug 12889) Florian Weimer via Libc-alpha
1 sibling, 1 reply; 6+ messages in thread
From: Florian Weimer via Libc-alpha @ 2021-09-06 12:33 UTC (permalink / raw)
To: libc-alpha
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
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)
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-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"
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>
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] nptl: Fix race between pthread_kill and thread exit (bug 12889)
2021-09-06 12:33 [PATCH v3 0/2] Fix pthread_cancel, pthread_kill race (bug 12889) Florian Weimer via Libc-alpha
2021-09-06 12:33 ` [PATCH v3 1/2] nptl: pthread_kill, pthread_cancel should not fail after exit (bug 19193) Florian Weimer via Libc-alpha
@ 2021-09-06 12:34 ` Florian Weimer via Libc-alpha
2021-09-07 13:21 ` Adhemerval Zanella via Libc-alpha
1 sibling, 1 reply; 6+ messages in thread
From: Florian Weimer via Libc-alpha @ 2021-09-06 12:34 UTC (permalink / raw)
To: libc-alpha
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
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>
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] nptl: pthread_kill, pthread_cancel should not fail after exit (bug 19193)
2021-09-06 12:33 ` [PATCH v3 1/2] nptl: pthread_kill, pthread_cancel should not fail after exit (bug 19193) Florian Weimer via Libc-alpha
@ 2021-09-07 12:42 ` Adhemerval Zanella via Libc-alpha
0 siblings, 0 replies; 6+ messages in thread
From: Adhemerval Zanella via Libc-alpha @ 2021-09-07 12:42 UTC (permalink / raw)
To: libc-alpha, Florian Weimer
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] nptl: Fix race between pthread_kill and thread exit (bug 12889)
2021-09-06 12:34 ` [PATCH v3 2/2] nptl: Fix race between pthread_kill and thread exit (bug 12889) Florian Weimer via Libc-alpha
@ 2021-09-07 13:21 ` Adhemerval Zanella via Libc-alpha
2021-09-10 15:19 ` Florian Weimer via Libc-alpha
0 siblings, 1 reply; 6+ messages in thread
From: Adhemerval Zanella via Libc-alpha @ 2021-09-07 13:21 UTC (permalink / raw)
To: libc-alpha, Florian Weimer
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] nptl: Fix race between pthread_kill and thread exit (bug 12889)
2021-09-07 13:21 ` Adhemerval Zanella via Libc-alpha
@ 2021-09-10 15:19 ` Florian Weimer via Libc-alpha
0 siblings, 0 replies; 6+ messages in thread
From: Florian Weimer via Libc-alpha @ 2021-09-10 15:19 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha
* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-10 15:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-06 12:33 [PATCH v3 0/2] Fix pthread_cancel, pthread_kill race (bug 12889) Florian Weimer via Libc-alpha
2021-09-06 12:33 ` [PATCH v3 1/2] nptl: pthread_kill, pthread_cancel should not fail after exit (bug 19193) Florian Weimer via Libc-alpha
2021-09-07 12:42 ` Adhemerval Zanella via Libc-alpha
2021-09-06 12:34 ` [PATCH v3 2/2] nptl: Fix race between pthread_kill and thread exit (bug 12889) Florian Weimer via Libc-alpha
2021-09-07 13:21 ` Adhemerval Zanella via Libc-alpha
2021-09-10 15:19 ` Florian Weimer via Libc-alpha
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).