* [PATCH 0/6] More NPTL fixes
@ 2021-06-10 19:36 Adhemerval Zanella
2021-06-10 19:36 ` [PATCH 1/6] nptl: Fix tst-cancel7 and tst-cancelx7 race condition (BZ #14232) Adhemerval Zanella
` (5 more replies)
0 siblings, 6 replies; 11+ messages in thread
From: Adhemerval Zanella @ 2021-06-10 19:36 UTC (permalink / raw)
To: libc-alpha
This patch fixes a couple of NPTL issues and continue to refactor the
cancellation code to simplify the required synchronization.
Adhemerval Zanella (6):
nptl: Fix tst-cancel7 and tst-cancelx7 race condition (BZ #14232)
nptl: Set cancellation type and state on pthread_exit
nptl: Handle robust PI mutexes for !__ASSUME_SET_ROBUST_LIST
nptl: Do not use pthread set_tid_address as state synchronization (BZ
#19951)
nptl: Move setxid flag out of cancelhandling
nptl: Replace struct thread cancelhandling field
nptl/Makefile | 5 +-
nptl/allocatestack.c | 5 +-
nptl/cancellation.c | 17 ++-
nptl/descr.h | 44 ++++----
nptl/nptl-stack.h | 2 +-
nptl/nptl_free_tcb.c | 22 ++--
nptl/nptl_setxid.c | 49 ++-------
nptl/pthreadP.h | 21 ++--
nptl/pthread_cancel.c | 11 +-
nptl/pthread_clockjoin.c | 2 +-
nptl/pthread_create.c | 111 +++++++++++---------
nptl/pthread_detach.c | 36 +++----
nptl/pthread_exit.c | 4 +-
nptl/pthread_getattr_np.c | 2 +-
nptl/pthread_join.c | 2 +-
nptl/pthread_join_common.c | 127 ++++++++--------------
nptl/pthread_testcancel.c | 11 +-
nptl/pthread_timedjoin.c | 2 +-
nptl/pthread_tryjoin.c | 18 ++--
nptl/tst-cancel7.c | 96 ++++++++---------
nptl/tst-cleanup5.c | 157 ++++++++++++++++++++++++++++
nptl_db/structs.def | 2 +-
nptl_db/td_thr_get_info.c | 16 ++-
nptl_db/td_thr_getfpregs.c | 9 +-
nptl_db/td_thr_getgregs.c | 9 +-
nptl_db/td_thr_setfpregs.c | 9 +-
nptl_db/td_thr_setgregs.c | 9 +-
sysdeps/hppa/nptl/tcb-offsets.sym | 1 -
sysdeps/i386/nptl/tcb-offsets.sym | 1 -
sysdeps/nptl/dl-tls_init_tp.c | 4 +-
sysdeps/nptl/libc_start_call_main.h | 7 ++
sysdeps/pthread/tst-thrd-detach.c | 16 ++-
sysdeps/sh/nptl/tcb-offsets.sym | 1 -
sysdeps/x86_64/nptl/tcb-offsets.sym | 4 -
34 files changed, 448 insertions(+), 384 deletions(-)
create mode 100644 nptl/tst-cleanup5.c
--
2.30.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/6] nptl: Fix tst-cancel7 and tst-cancelx7 race condition (BZ #14232)
2021-06-10 19:36 [PATCH 0/6] More NPTL fixes Adhemerval Zanella
@ 2021-06-10 19:36 ` Adhemerval Zanella
2021-06-11 15:02 ` Florian Weimer
2021-06-10 19:36 ` [PATCH 2/6] nptl: Set cancellation type and state on pthread_exit Adhemerval Zanella
` (4 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Adhemerval Zanella @ 2021-06-10 19:36 UTC (permalink / raw)
To: libc-alpha
A named semaphore is used to synchronize the pid information on the
created file, the semaphore is updated once the file contents is
flushed.
Checked on x86_64-linux-gnu.
---
nptl/Makefile | 2 +
nptl/tst-cancel7.c | 96 +++++++++++++++++++++-------------------------
2 files changed, 45 insertions(+), 53 deletions(-)
diff --git a/nptl/Makefile b/nptl/Makefile
index 3e6cf0c21b..0ad9e24b0f 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -539,6 +539,8 @@ else
librt = $(common-objpfx)rt/librt.a
endif
+$(objpfx)tst-cancel7: $(librt)
+$(objpfx)tst-cancelx7: $(librt)
$(objpfx)tst-cancel17: $(librt)
$(objpfx)tst-cancelx17: $(librt)
diff --git a/nptl/tst-cancel7.c b/nptl/tst-cancel7.c
index 7a1870ac74..e69fa7e7af 100644
--- a/nptl/tst-cancel7.c
+++ b/nptl/tst-cancel7.c
@@ -18,44 +18,45 @@
#include <errno.h>
#include <fcntl.h>
-#include <pthread.h>
+#include <getopt.h>
#include <signal.h>
-#include <stdio.h>
#include <stdlib.h>
-#include <string.h>
-#include <unistd.h>
-#include <getopt.h>
+#include <semaphore.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/xstdio.h>
+#include <support/xunistd.h>
#include <support/xthread.h>
-const char *command;
-const char *pidfile;
-char pidfilename[] = "/tmp/tst-cancel7-XXXXXX";
+static const char *command;
+static const char *pidfile;
+static char *pidfilename;
+
+static const char sem_name[] = "/tst-cancel7-sem";
+static sem_t *sem;
static void *
tf (void *arg)
{
- const char *args = " --direct --pidfile ";
- char *cmd = alloca (strlen (command) + strlen (args)
- + strlen (pidfilename) + 1);
-
- strcpy (stpcpy (stpcpy (cmd, command), args), pidfilename);
+ char *cmd = xasprintf ("%s --direct --pidfile %s", command, pidfilename);
system (cmd);
/* This call should never return. */
return NULL;
}
-
static void
sl (void)
{
- FILE *f = fopen (pidfile, "w");
- if (f == NULL)
- exit (1);
+ FILE *f = xfopen (pidfile, "w");
fprintf (f, "%lld\n", (long long) getpid ());
fflush (f);
+ if (sem_post (sem) != 0)
+ FAIL_EXIT1 ("sem_post: %m");
+
struct flock fl =
{
.l_type = F_WRLCK,
@@ -64,7 +65,7 @@ sl (void)
.l_len = 1
};
if (fcntl (fileno (f), F_SETLK, &fl) != 0)
- exit (1);
+ FAIL_EXIT1 ("fcntl (F_SETFL): %m");
sigset_t ss;
sigfillset (&ss);
@@ -76,57 +77,51 @@ sl (void)
static void
do_prepare (int argc, char *argv[])
{
+ sem = sem_open (sem_name, O_RDWR | O_CREAT | O_EXCL | O_TRUNC, 0666, 0);
+ if (sem == SEM_FAILED)
+ {
+ if (errno != EEXIST)
+ FAIL_EXIT1 ("sem_open failed: %m");
+ sem = sem_open (sem_name, O_RDWR);
+ if (sem == SEM_FAILED)
+ FAIL_EXIT1 ("sem_open failed: %m");
+ }
+
if (command == NULL)
command = argv[0];
if (pidfile)
sl ();
- int fd = mkstemp (pidfilename);
+ int fd = create_temp_file ("tst-cancel7-pid-", &pidfilename);
if (fd == -1)
- {
- puts ("mkstemp failed");
- exit (1);
- }
+ FAIL_EXIT1 ("create_temp_file failed: %m");
- write (fd, " ", 1);
- close (fd);
+ xwrite (fd, " ", 1);
+ xclose (fd);
}
static int
do_test (void)
{
- pthread_t th;
- if (pthread_create (&th, NULL, tf, NULL) != 0)
- {
- puts ("pthread_create failed");
- return 1;
- }
+ pthread_t th = xpthread_create (NULL, tf, NULL);
do
- sleep (1);
+ nanosleep (&(struct timespec) { .tv_sec = 0, .tv_nsec = 100000000 }, NULL);
while (access (pidfilename, R_OK) != 0);
xpthread_cancel (th);
void *r = xpthread_join (th);
- sleep (1);
+ if (sem_wait (sem) != 0)
+ FAIL_EXIT1 ("sem_wait: %m");
- FILE *f = fopen (pidfilename, "r+");
- if (f == NULL)
- {
- puts ("no pidfile");
- return 1;
- }
+ FILE *f = xfopen (pidfilename, "r+");
long long ll;
if (fscanf (f, "%lld\n", &ll) != 1)
- {
- puts ("could not read pid");
- unlink (pidfilename);
- return 1;
- }
+ FAIL_EXIT1 ("fscanf: %m");
struct flock fl =
{
@@ -136,11 +131,7 @@ do_test (void)
.l_len = 1
};
if (fcntl (fileno (f), F_GETLK, &fl) != 0)
- {
- puts ("F_GETLK failed");
- unlink (pidfilename);
- return 1;
- }
+ FAIL_EXIT1 ("fcntl: %m");
if (fl.l_type != F_UNLCK)
{
@@ -148,13 +139,12 @@ do_test (void)
if (fl.l_pid == ll)
kill (fl.l_pid, SIGKILL);
- unlink (pidfilename);
return 1;
}
- fclose (f);
+ xfclose (f);
- unlink (pidfilename);
+ sem_unlink (sem_name);
return r != PTHREAD_CANCELED;
}
@@ -181,7 +171,7 @@ do_cleanup (void)
fclose (f);
}
- unlink (pidfilename);
+ sem_unlink (sem_name);
}
#define OPT_COMMAND 10000
--
2.30.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/6] nptl: Set cancellation type and state on pthread_exit
2021-06-10 19:36 [PATCH 0/6] More NPTL fixes Adhemerval Zanella
2021-06-10 19:36 ` [PATCH 1/6] nptl: Fix tst-cancel7 and tst-cancelx7 race condition (BZ #14232) Adhemerval Zanella
@ 2021-06-10 19:36 ` Adhemerval Zanella
2021-06-10 19:36 ` [PATCH 3/6] nptl: Handle robust PI mutexes for !__ASSUME_SET_ROBUST_LIST Adhemerval Zanella
` (3 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Adhemerval Zanella @ 2021-06-10 19:36 UTC (permalink / raw)
To: libc-alpha
It is required by POSIX XSH 2.9.5 Thread Cancellation under the heading
Thread Cancellation Cleanup Handlers.
Checked x86_64-linux-gnu.
---
nptl/Makefile | 3 +-
nptl/cancellation.c | 7 +-
nptl/pthreadP.h | 18 ++++-
nptl/pthread_cancel.c | 1 -
nptl/pthread_exit.c | 4 +-
nptl/pthread_testcancel.c | 1 -
nptl/tst-cleanup5.c | 157 ++++++++++++++++++++++++++++++++++++++
7 files changed, 180 insertions(+), 11 deletions(-)
create mode 100644 nptl/tst-cleanup5.c
diff --git a/nptl/Makefile b/nptl/Makefile
index 0ad9e24b0f..0dd139a53d 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -307,7 +307,8 @@ tests = tst-attr2 tst-attr3 tst-default-attr \
tst-pthread-gdb-attach tst-pthread-gdb-attach-static \
tst-pthread_exit-nothreads \
tst-pthread_exit-nothreads-static \
- tst-thread-setspecific
+ tst-thread-setspecific \
+ tst-cleanup5
tests-nolibpthread = \
tst-pthread_exit-nothreads \
diff --git a/nptl/cancellation.c b/nptl/cancellation.c
index 05962784d5..6478c029de 100644
--- a/nptl/cancellation.c
+++ b/nptl/cancellation.c
@@ -42,7 +42,6 @@ __pthread_enable_asynccancel (void)
&& !(ch & EXITING_BITMASK)
&& !(ch & TERMINATED_BITMASK))
{
- THREAD_SETMEM (self, result, PTHREAD_CANCELED);
__do_cancel ();
}
@@ -64,3 +63,9 @@ __pthread_disable_asynccancel (int oldtype)
self->canceltype = PTHREAD_CANCEL_DEFERRED;
}
libc_hidden_def (__pthread_disable_asynccancel)
+
+void
+__do_cancel (void)
+{
+ __exit_thread (PTHREAD_CANCELED);
+}
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index 675d1de753..ef0b367120 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -268,20 +268,30 @@ extern void __pthread_unregister_cancel (__pthread_unwind_buf_t *__buf)
__cleanup_fct_attribute;
libc_hidden_proto (__pthread_unregister_cancel)
-/* Called when a thread reacts on a cancellation request. */
-static inline void
-__attribute ((noreturn, always_inline))
-__do_cancel (void)
+static _Noreturn inline void
+__exit_thread (void *value)
{
struct pthread *self = THREAD_SELF;
/* Make sure we get no more cancellations. */
THREAD_ATOMIC_BIT_SET (self, cancelhandling, EXITING_BIT);
+ THREAD_SETMEM (self, result, value);
+
+ /* It is required by POSIX XSH 2.9.5 Thread Cancellation under the heading
+ Thread Cancellation Cleanup Handlers and also avoid further cancellation
+ wrapper to act on cancellation. */
+ THREAD_SETMEM (self, cancelstate, PTHREAD_CANCEL_DISABLE);
+ THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_DEFERRED);
+
__pthread_unwind ((__pthread_unwind_buf_t *)
THREAD_GETMEM (self, cleanup_jmp_buf));
}
+/* It is a wrapper over __exit_thread (PTHREAD_CANCELED). It is has its own
+ implementation because it might be called by arch-specific asm code. */
+_Noreturn void __do_cancel (void) attribute_hidden;
+
/* Internal prototypes. */
diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
index 0698cd2046..577ff6c746 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -103,7 +103,6 @@ __pthread_cancel (pthread_t th)
__libc_multiple_threads = 1;
#endif
- THREAD_SETMEM (pd, result, PTHREAD_CANCELED);
if (pd->cancelstate == PTHREAD_CANCEL_ENABLE
&& pd->canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
__do_cancel ();
diff --git a/nptl/pthread_exit.c b/nptl/pthread_exit.c
index 6abf66463e..978f4042ef 100644
--- a/nptl/pthread_exit.c
+++ b/nptl/pthread_exit.c
@@ -32,9 +32,7 @@ __pthread_exit (void *value)
" must be installed for pthread_exit to work\n");
}
- THREAD_SETMEM (THREAD_SELF, result, value);
-
- __do_cancel ();
+ __exit_thread (value);
}
libc_hidden_def (__pthread_exit)
weak_alias (__pthread_exit, pthread_exit)
diff --git a/nptl/pthread_testcancel.c b/nptl/pthread_testcancel.c
index 920374643a..e6adf12580 100644
--- a/nptl/pthread_testcancel.c
+++ b/nptl/pthread_testcancel.c
@@ -30,7 +30,6 @@ ___pthread_testcancel (void)
&& !(cancelhandling & EXITING_BITMASK)
&& !(cancelhandling & TERMINATED_BITMASK))
{
- THREAD_SETMEM (self, result, PTHREAD_CANCELED);
__do_cancel ();
}
}
diff --git a/nptl/tst-cleanup5.c b/nptl/tst-cleanup5.c
new file mode 100644
index 0000000000..6cc1c141e9
--- /dev/null
+++ b/nptl/tst-cleanup5.c
@@ -0,0 +1,157 @@
+/* Check if cancellation state and type is correctly set on 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/>. */
+
+#include <support/check.h>
+#include <support/xthread.h>
+#include <support/xunistd.h>
+
+static int pipefds[2];
+static pthread_barrier_t b;
+
+static void
+clh (void *arg)
+{
+ /* Although POSIX state setting either the cancellation state or type is
+ undefined during cleanup handler execution, both calls should be safe
+ since none has any side-effect (they should not change current state
+ neither trigger a pending cancellation). */
+
+ int state;
+ TEST_VERIFY (pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, &state) == 0);
+ TEST_COMPARE (state, PTHREAD_CANCEL_DISABLE);
+
+ int type;
+ TEST_VERIFY (pthread_setcanceltype (PTHREAD_CANCEL_DEFERRED, &type) == 0);
+ TEST_COMPARE (type, PTHREAD_CANCEL_DEFERRED);
+}
+
+/* Check if a thread with PTHREAD_CANCEL_DEFERRED cancellation on
+ pthread_cleanup_pop sets the correct state and type as pthread_exit. */
+static void *
+tf_cancel_deferred (void *arg)
+{
+ xpthread_barrier_wait (&b);
+
+ pthread_cleanup_push (clh, NULL);
+
+ char c;
+ read (pipefds[0], &c, 1);
+
+ pthread_cleanup_pop (1);
+
+ return NULL;
+}
+
+/* Check if a thread with PTHREAD_CANCEL_ASYNCHRONOUS cancellation on
+ blocked read() sets the correct state and type as pthread_exit. */
+static void *
+tf_cancel_async (void *arg)
+{
+ TEST_VERIFY (pthread_setcanceltype (PTHREAD_CANCEL_ASYNCHRONOUS, NULL)
+ == 0);
+
+ xpthread_barrier_wait (&b);
+
+ pthread_cleanup_push (clh, NULL);
+
+ char c;
+ read (pipefds[0], &c, 1);
+
+ pthread_cleanup_pop (1);
+
+ return NULL;
+}
+
+/* Check if a thread with PTHREAD_CANCEL_DEFERRED cancellation on
+ blocked read() sets the correct state and type as pthread_exit. */
+static void *
+tf_testcancel (void *arg)
+{
+ xpthread_barrier_wait (&b);
+
+ pthread_cleanup_push (clh, NULL);
+
+ char c;
+ read (pipefds[0], &c, 1);
+
+ pthread_testcancel ();
+
+ pthread_cleanup_pop (1);
+
+ return NULL;
+}
+
+#define EXIT_EXPECTED_VALUE ((void *) 42)
+
+/* Check if a thread with PTHREAD_CANCEL_DEFERRED cancellation on
+ pthread_exit() sets the correct state and type. */
+static void *
+tf_exit (void *arg)
+{
+ xpthread_barrier_wait (&b);
+
+ pthread_cleanup_push (clh, NULL);
+
+ pthread_exit (EXIT_EXPECTED_VALUE);
+
+ pthread_cleanup_pop (1);
+
+ return NULL;
+}
+
+static int
+do_test (void)
+{
+ xpipe (pipefds);
+
+ xpthread_barrier_init (&b, NULL, 2);
+ {
+ pthread_t th = xpthread_create (NULL, tf_cancel_deferred, NULL);
+ xpthread_barrier_wait (&b);
+ xpthread_cancel (th);
+ void *r = xpthread_join (th);
+ TEST_VERIFY (r == PTHREAD_CANCELED);
+ }
+
+ {
+ pthread_t th = xpthread_create (NULL, tf_cancel_async, NULL);
+ xpthread_barrier_wait (&b);
+ xpthread_cancel (th);
+ void *r = xpthread_join (th);
+ TEST_VERIFY (r == PTHREAD_CANCELED);
+ }
+
+ {
+ pthread_t th = xpthread_create (NULL, tf_testcancel, NULL);
+ xpthread_barrier_wait (&b);
+ xpthread_cancel (th);
+ void *r = xpthread_join (th);
+ TEST_VERIFY (r == PTHREAD_CANCELED);
+ }
+
+ {
+ pthread_t th = xpthread_create (NULL, tf_exit, NULL);
+ xpthread_barrier_wait (&b);
+ void *r = xpthread_join (th);
+ TEST_VERIFY (r == EXIT_EXPECTED_VALUE);
+ }
+
+ return 0;
+}
+
+#include <support/test-driver.c>
--
2.30.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/6] nptl: Handle robust PI mutexes for !__ASSUME_SET_ROBUST_LIST
2021-06-10 19:36 [PATCH 0/6] More NPTL fixes Adhemerval Zanella
2021-06-10 19:36 ` [PATCH 1/6] nptl: Fix tst-cancel7 and tst-cancelx7 race condition (BZ #14232) Adhemerval Zanella
2021-06-10 19:36 ` [PATCH 2/6] nptl: Set cancellation type and state on pthread_exit Adhemerval Zanella
@ 2021-06-10 19:36 ` Adhemerval Zanella
2021-06-10 19:36 ` [PATCH 4/6] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951) Adhemerval Zanella
` (2 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Adhemerval Zanella @ 2021-06-10 19:36 UTC (permalink / raw)
To: libc-alpha
The robust PI mutexes are signaled by setting the LSB bit to 1, so
the code requires to take this consideration before access the
__pthread_mutex_s.
The code is also simplified: the initialization code is not really
required, PD->robust_head.list and PD->robust_list.__next are
essentially the same regardless of __PTHREAD_MUTEX_HAVE_PREV, the futex
wake is optimized to be issued only when required, and the futex shared
bit is set only when required.
Checked on a build for m68k-linux-gnu. I also checked on
x86_64-linux-gnu by removing the check for !__ASSUME_SET_ROBUST_LIST.
---
nptl/pthread_create.c | 53 ++++++++++++++++++++++---------------------
1 file changed, 27 insertions(+), 26 deletions(-)
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index 3f017f1e26..8f1a77b5a8 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -478,35 +478,36 @@ start_thread (void *arg)
exit (0);
#ifndef __ASSUME_SET_ROBUST_LIST
- /* If this thread has any robust mutexes locked, handle them now. */
-# if __PTHREAD_MUTEX_HAVE_PREV
- void *robust = pd->robust_head.list;
-# else
- __pthread_slist_t *robust = pd->robust_list.__next;
-# endif
- /* We let the kernel do the notification if it is able to do so.
- If we have to do it here there for sure are no PI mutexes involved
- since the kernel support for them is even more recent. */
- if (!__nptl_set_robust_list_avail
- && __builtin_expect (robust != (void *) &pd->robust_head, 0))
+ /* We let the kernel do the notification if it is able to do so on the exit
+ syscall. Otherwise we need to handle before the thread terminates. */
+ void **robust;
+ while ((robust = pd->robust_head.list)
+ && robust != (void *) &pd->robust_head)
{
- do
+ /* Note: robust PI futexes are signaled by setting bit 0. */
+ void **robustp = (void **) ((uintptr_t) robust & ~1UL);
+
+ struct __pthread_mutex_s *mtx = (struct __pthread_mutex_s *)
+ ((char *) robustp - offsetof (struct __pthread_mutex_s,
+ __list.__next));
+ unsigned int nusers = mtx->__nusers;
+ int shared = mtx->__kind & 128;
+
+ pd->robust_head.list_op_pending = robust;
+ pd->robust_head.list = *robustp;
+ /* Although the list will not be changed at this point, it follows the
+ expected kernel ABI. */
+ __asm ("" ::: "memory");
+
+ int lock = atomic_exchange_relaxed (&mtx->__lock, FUTEX_OWNER_DIED);
+ /* Wake any users if mutex is acquired with potential users. */
+ if (lock > 1 || nusers != 0)
{
- struct __pthread_mutex_s *this = (struct __pthread_mutex_s *)
- ((char *) robust - offsetof (struct __pthread_mutex_s,
- __list.__next));
- robust = *((void **) robust);
-
-# if __PTHREAD_MUTEX_HAVE_PREV
- this->__list.__prev = NULL;
-# endif
- this->__list.__next = NULL;
-
- atomic_or (&this->__lock, FUTEX_OWNER_DIED);
- futex_wake ((unsigned int *) &this->__lock, 1,
- /* XYZ */ FUTEX_SHARED);
+ if ((uintptr_t) robust & 1)
+ futex_unlock_pi ((unsigned int *) &mtx->__lock, shared);
+ else
+ futex_wake ((unsigned int *) &mtx->__lock, 1, shared);
}
- while (robust != (void *) &pd->robust_head);
}
#endif
--
2.30.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/6] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951)
2021-06-10 19:36 [PATCH 0/6] More NPTL fixes Adhemerval Zanella
` (2 preceding siblings ...)
2021-06-10 19:36 ` [PATCH 3/6] nptl: Handle robust PI mutexes for !__ASSUME_SET_ROBUST_LIST Adhemerval Zanella
@ 2021-06-10 19:36 ` Adhemerval Zanella
2021-06-10 19:36 ` [PATCH 5/6] nptl: Move setxid flag out of cancelhandling Adhemerval Zanella
2021-06-10 19:36 ` [PATCH 6/6] nptl: Replace struct thread cancelhandling field Adhemerval Zanella
5 siblings, 0 replies; 11+ messages in thread
From: Adhemerval Zanella @ 2021-06-10 19:36 UTC (permalink / raw)
To: libc-alpha
The use after free described in BZ#19951 is due the use of two different
PD fields, 'joinid' and 'cancelhandling', to describe the thread state.
And any state change potentially requires to check for both fields
atomically to potentially handle partial state (such as pthread_join()
with a cancellation handler and a joinstate rollback).
This patch uses a different PD member with 4 possible states (JOINABLE,
DETACHED, EXITING, and EXITED) to synchronize between pthread_create(),
pthread_join(), pthread_detached(), and pthread_exit():
1. On pthread_create() the inital state is set either to JOINABLE or
DETACHED depending of the pthread attribute used.
2. On pthread_detach() call, a CAS is issued on the state. If the CAS
fails it means that thread is already detached (DETACHED) or is
being terminated (EXITING). For former an EINVAL is returned,
while for latter pthread_detach() should be reponsible to join the
thread (and deallocate the resources).
3. On pthread_create() exit phase (reached either if the thread
function has returned, pthread_exit() has being called, or
cancellation handled has been acted upon) we issue a CAS on state
to set to EXITING Mode. If the thread is previous on DETACHED
mode it will be the thread itself responsible to deallocate any
resource, otherwise the threads needs to be joined.
4. The clear_tid_field on 'clone' call is changed to set the new
'state' field on thread exit (EXITED). This state ins only
reached on thread termination.
4. The pthread_join() implementation is now simpler: the futex wait
is done directly on thread state and there is no need to reset it
in case of timeout (since the state is now set either by
pthread_detach() or by the kernel on process termination).
The race condition on pthread_detach is avoided with only one atomic
operation on PD state: once the mode is set to THREAD_STATE_DETACHED
it is up to thread itself to deallocate its memory (done on the exit
phase at pthread_create()).
This change trigger an invalid C11 thread tests: it crates a thread,
which detaches itself, and after a timeout the creating thread checks
if the join fails. The issue is once thrd_join() is called the thread
lifetime has already expired. The test is changed so the slee is done
by the thread itself, so the creating thread will try to join a valid
thread.
Checked on x86_64-linux-gnu, i686-linux-gnu, aarch64-linux-gnu,
arm-linux-gnueabihf, and powerpc64-linux-gnu.
---
nptl/descr.h | 26 +++---
nptl/nptl-stack.h | 2 +-
nptl/pthreadP.h | 2 +-
nptl/pthread_clockjoin.c | 2 +-
nptl/pthread_create.c | 48 ++++++----
nptl/pthread_detach.c | 36 +++-----
nptl/pthread_getattr_np.c | 2 +-
nptl/pthread_join.c | 2 +-
nptl/pthread_join_common.c | 131 ++++++++++------------------
nptl/pthread_timedjoin.c | 2 +-
nptl/pthread_tryjoin.c | 18 ++--
sysdeps/nptl/dl-tls_init_tp.c | 4 +-
sysdeps/nptl/libc_start_call_main.h | 7 ++
sysdeps/pthread/tst-thrd-detach.c | 16 ++--
14 files changed, 138 insertions(+), 160 deletions(-)
diff --git a/nptl/descr.h b/nptl/descr.h
index c85778d449..4b2db6edab 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -124,6 +124,18 @@ struct priority_protection_data
};
+/* Define a possible thread state on 'joinstate' field. The value will be
+ cleared by the kernel when the thread terminates (CLONE_CHILD_CLEARTID),
+ so THREAD_STATE_EXITED must be 0. */
+enum
+ {
+ THREAD_STATE_EXITED = 0,
+ THREAD_STATE_EXITING,
+ THREAD_STATE_JOINABLE,
+ THREAD_STATE_DETACHED,
+ };
+
+
/* Thread descriptor data structure. */
struct pthread
{
@@ -166,8 +178,7 @@ struct pthread
GL (dl_stack_user) list. */
list_t list;
- /* Thread ID - which is also a 'is this thread descriptor (and
- therefore stack) used' flag. */
+ /* Thread ID set by the kernel with CLONE_PARENT_SETTID. */
pid_t tid;
/* Ununsed. */
@@ -335,15 +346,8 @@ struct pthread
hp_timing_t cpuclock_offset_ununsed;
#endif
- /* If the thread waits to join another one the ID of the latter is
- stored here.
-
- In case a thread is detached this field contains a pointer of the
- TCB if the thread itself. This is something which cannot happen
- in normal operation. */
- struct pthread *joinid;
- /* Check whether a thread is detached. */
-#define IS_DETACHED(pd) ((pd)->joinid == (pd))
+ /* The current thread state defined by the THREAD_STATE_* enumeration. */
+ unsigned int joinstate;
/* The result of the thread function. */
void *result;
diff --git a/nptl/nptl-stack.h b/nptl/nptl-stack.h
index a6bd8df77f..8f3cfbf71e 100644
--- a/nptl/nptl-stack.h
+++ b/nptl/nptl-stack.h
@@ -29,7 +29,7 @@
static inline bool
__nptl_stack_in_use (struct pthread *pd)
{
- return pd->tid <= 0;
+ return atomic_load_relaxed (&pd->joinstate) == THREAD_STATE_EXITED;
}
/* Remove the stack ELEM from its list. */
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index ef0b367120..bfb0e40b44 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -536,7 +536,7 @@ libc_hidden_proto (__pthread_setcanceltype)
extern void __pthread_testcancel (void);
libc_hidden_proto (__pthread_testcancel)
extern int __pthread_clockjoin_ex (pthread_t, void **, clockid_t,
- const struct __timespec64 *, bool)
+ const struct __timespec64 *)
attribute_hidden;
extern int __pthread_sigmask (int, const sigset_t *, sigset_t *);
libc_hidden_proto (__pthread_sigmask);
diff --git a/nptl/pthread_clockjoin.c b/nptl/pthread_clockjoin.c
index f5007d7831..8798f26fcf 100644
--- a/nptl/pthread_clockjoin.c
+++ b/nptl/pthread_clockjoin.c
@@ -30,7 +30,7 @@ ___pthread_clockjoin_np64 (pthread_t threadid, void **thread_return,
return EINVAL;
return __pthread_clockjoin_ex (threadid, thread_return,
- clockid, abstime, true);
+ clockid, abstime);
}
#if __TIMESIZE == 64
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index 8f1a77b5a8..0c0f2c03e5 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -283,7 +283,8 @@ static int create_thread (struct pthread *pd, const struct pthread_attr *attr,
# define ARCH_CLONE __clone
#endif
if (__glibc_unlikely (ARCH_CLONE (&start_thread, STACK_VARIABLES_ARGS,
- clone_flags, pd, &pd->tid, tp, &pd->tid)
+ clone_flags, pd, &pd->tid, tp,
+ &pd->joinstate)
== -1))
return errno;
@@ -343,7 +344,7 @@ start_thread (void *arg)
and free any resource prior return to the pthread_create caller. */
setup_failed = pd->setup_failed == 1;
if (setup_failed)
- pd->joinid = NULL;
+ pd->joinstate = THREAD_STATE_JOINABLE;
/* And give it up right away. */
lll_unlock (pd->lock, LLL_PRIVATE);
@@ -473,9 +474,20 @@ start_thread (void *arg)
the breakpoint reports TD_THR_RUN state rather than TD_THR_ZOMBIE. */
atomic_bit_set (&pd->cancelhandling, EXITING_BIT);
- if (__glibc_unlikely (atomic_decrement_and_test (&__nptl_nthreads)))
- /* This was the last thread. */
- exit (0);
+
+ /* CONCURRENCY NOTES:
+
+ Concurrent pthread_detach() will either set state to
+ THREAD_STATE_DETACHED or wait the thread to terminate. The exiting state
+ set here is set so a pthread_join() wait until all the required cleanup
+ steps are done.
+
+ The 'joinstate' field will be used to determine who is responsible to
+ call __nptl_free_tcb below. */
+
+ unsigned int joinstate = THREAD_STATE_JOINABLE;
+ atomic_compare_exchange_weak_acquire (&pd->joinstate, &joinstate,
+ THREAD_STATE_EXITING);
#ifndef __ASSUME_SET_ROBUST_LIST
/* We let the kernel do the notification if it is able to do so on the exit
@@ -511,6 +523,10 @@ start_thread (void *arg)
}
#endif
+ if (__glibc_unlikely (atomic_decrement_and_test (&__nptl_nthreads)))
+ /* This was the last thread. */
+ exit (0);
+
if (!pd->user_stack)
advise_stack_range (pd->stackblock, pd->stackblock_size, (uintptr_t) pd,
pd->guardsize);
@@ -531,9 +547,7 @@ start_thread (void *arg)
pd->setxid_futex = 0;
}
- /* If the thread is detached free the TCB. */
- if (IS_DETACHED (pd))
- /* Free the TCB. */
+ if (joinstate == THREAD_STATE_DETACHED)
__nptl_free_tcb (pd);
out:
@@ -541,7 +555,7 @@ out:
The 'exit' implementation in the kernel will signal when the
process is really dead since 'clone' got passed the CLONE_CHILD_CLEARTID
- flag. The 'tid' field in the TCB will be set to zero.
+ flag. The 'joinstate' field in the TCB will be set to zero.
The exit code is zero since in case all threads exit by calling
'pthread_exit' the exit status must be 0 (zero). */
@@ -635,10 +649,9 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
pd->flags = ((iattr->flags & ~(ATTR_FLAG_SCHED_SET | ATTR_FLAG_POLICY_SET))
| (self->flags & (ATTR_FLAG_SCHED_SET | ATTR_FLAG_POLICY_SET)));
- /* Initialize the field for the ID of the thread which is waiting
- for us. This is a self-reference in case the thread is created
- detached. */
- pd->joinid = iattr->flags & ATTR_FLAG_DETACHSTATE ? pd : NULL;
+ pd->joinstate = iattr->flags & ATTR_FLAG_DETACHSTATE
+ ? THREAD_STATE_DETACHED
+ : THREAD_STATE_JOINABLE;
/* The debug events are inherited from the parent. */
pd->eventbuf = self->eventbuf;
@@ -797,10 +810,11 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
/* Similar to pthread_join, but since thread creation has failed at
startup there is no need to handle all the steps. */
- pid_t tid;
- while ((tid = atomic_load_acquire (&pd->tid)) != 0)
- __futex_abstimed_wait_cancelable64 ((unsigned int *) &pd->tid,
- tid, 0, NULL, LLL_SHARED);
+ unsigned int state;
+ while ((state = atomic_load_acquire (&pd->joinstate))
+ != THREAD_STATE_EXITED)
+ __futex_abstimed_wait_cancelable64 (&pd->joinstate, state, 0,
+ NULL, LLL_SHARED);
}
/* State (c) or (d) and we have ownership of PD (see CONCURRENCY
diff --git a/nptl/pthread_detach.c b/nptl/pthread_detach.c
index ac50db9b0e..97d292cab8 100644
--- a/nptl/pthread_detach.c
+++ b/nptl/pthread_detach.c
@@ -26,32 +26,24 @@ ___pthread_detach (pthread_t th)
{
struct pthread *pd = (struct pthread *) th;
- /* Make sure the descriptor is valid. */
- if (INVALID_NOT_TERMINATED_TD_P (pd))
- /* Not a valid thread handle. */
- return ESRCH;
+ /* CONCURRENCY NOTES:
- int result = 0;
+ Concurrent pthread_detach() will return EINVAL for the case the thread
+ is already detached (THREAD_STATE_DETACHED). POSIX states it is
+ undefined to call pthread_detach if TH refers to a non joinable thread.
- /* Mark the thread as detached. */
- if (atomic_compare_and_exchange_bool_acq (&pd->joinid, pd, NULL))
+ For the case the thread is being terminated (THREAD_STATE_EXITING),
+ pthread_detach() will responsible to clean up the stack. */
+
+ int curstate = THREAD_STATE_JOINABLE;
+ if (!atomic_compare_exchange_weak_acquire (&pd->joinstate, &curstate,
+ THREAD_STATE_DETACHED))
{
- /* There are two possibilities here. First, the thread might
- already be detached. In this case we return EINVAL.
- Otherwise there might already be a waiter. The standard does
- not mention what happens in this case. */
- if (IS_DETACHED (pd))
- result = EINVAL;
+ if (curstate == THREAD_STATE_DETACHED)
+ return EINVAL;
+ return __pthread_join (th, 0);
}
- else
- /* Check whether the thread terminated meanwhile. In this case we
- will just free the TCB. */
- if ((pd->cancelhandling & EXITING_BITMASK) != 0)
- /* Note that the code in __free_tcb makes sure each thread
- control block is freed only once. */
- __nptl_free_tcb (pd);
-
- return result;
+ return 0;
}
versioned_symbol (libc, ___pthread_detach, pthread_detach, GLIBC_2_34);
libc_hidden_ver (___pthread_detach, __pthread_detach)
diff --git a/nptl/pthread_getattr_np.c b/nptl/pthread_getattr_np.c
index 25807cb529..560ba9ab98 100644
--- a/nptl/pthread_getattr_np.c
+++ b/nptl/pthread_getattr_np.c
@@ -53,7 +53,7 @@ __pthread_getattr_np (pthread_t thread_id, pthread_attr_t *attr)
iattr->flags = thread->flags;
/* The thread might be detached by now. */
- if (IS_DETACHED (thread))
+ if (atomic_load_acquire (&thread->joinstate) == THREAD_STATE_DETACHED)
iattr->flags |= ATTR_FLAG_DETACHSTATE;
/* This is the guardsize after adjusting it. */
diff --git a/nptl/pthread_join.c b/nptl/pthread_join.c
index d2b33de73d..195a537029 100644
--- a/nptl/pthread_join.c
+++ b/nptl/pthread_join.c
@@ -23,7 +23,7 @@ int
___pthread_join (pthread_t threadid, void **thread_return)
{
return __pthread_clockjoin_ex (threadid, thread_return, 0 /* Ignored */,
- NULL, true);
+ NULL);
}
versioned_symbol (libc, ___pthread_join, pthread_join, GLIBC_2_34);
libc_hidden_ver (___pthread_join, __pthread_join)
diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
index 7303069316..0870f36ecb 100644
--- a/nptl/pthread_join_common.c
+++ b/nptl/pthread_join_common.c
@@ -22,113 +22,74 @@
#include <time.h>
#include <futex-internal.h>
-static void
-cleanup (void *arg)
+/* Check for a possible deadlock situation where the threads are waiting for
+ each other to finish. Note that this is a "may" error. To be 100% sure we
+ catch this error we would have to lock the data structures but it is not
+ necessary. In the unlikely case that two threads are really caught in this
+ situation they will deadlock. It is the programmer's problem to figure
+ this out. */
+static inline bool
+check_for_deadlock (int state, struct pthread *pd)
{
- /* If we already changed the waiter ID, reset it. The call cannot
- fail for any reason but the thread not having done that yet so
- there is no reason for a loop. */
struct pthread *self = THREAD_SELF;
- atomic_compare_exchange_weak_acquire (&arg, &self, NULL);
+ return ((pd == self
+ || (atomic_load_acquire (&self->joinstate) == THREAD_STATE_DETACHED
+ && (pd->cancelhandling
+ & (CANCELED_BITMASK | EXITING_BITMASK
+ | TERMINATED_BITMASK)) == 0))
+ && !(self->cancelstate == PTHREAD_CANCEL_ENABLE
+ && (pd->cancelhandling & (CANCELED_BITMASK | EXITING_BITMASK
+ | TERMINATED_BITMASK))
+ == CANCELED_BITMASK));
}
int
__pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
clockid_t clockid,
- const struct __timespec64 *abstime, bool block)
+ const struct __timespec64 *abstime)
{
struct pthread *pd = (struct pthread *) threadid;
- /* Make sure the descriptor is valid. */
- if (INVALID_NOT_TERMINATED_TD_P (pd))
- /* Not a valid thread handle. */
- return ESRCH;
-
- /* Is the thread joinable?. */
- if (IS_DETACHED (pd))
- /* We cannot wait for the thread. */
- return EINVAL;
-
- struct pthread *self = THREAD_SELF;
- int result = 0;
-
LIBC_PROBE (pthread_join, 1, threadid);
- if ((pd == self
- || (self->joinid == pd
- && (pd->cancelhandling
- & (CANCELED_BITMASK | EXITING_BITMASK
- | TERMINATED_BITMASK)) == 0))
- && !(self->cancelstate == PTHREAD_CANCEL_ENABLE
- && (pd->cancelhandling & (CANCELED_BITMASK | EXITING_BITMASK
- | TERMINATED_BITMASK))
- == CANCELED_BITMASK))
- /* This is a deadlock situation. The threads are waiting for each
- other to finish. Note that this is a "may" error. To be 100%
- sure we catch this error we would have to lock the data
- structures but it is not necessary. In the unlikely case that
- two threads are really caught in this situation they will
- deadlock. It is the programmer's problem to figure this
- out. */
- return EDEADLK;
-
- /* Wait for the thread to finish. If it is already locked something
- is wrong. There can only be one waiter. */
- else if (__glibc_unlikely (atomic_compare_exchange_weak_acquire (&pd->joinid,
- &self,
- NULL)))
- /* There is already somebody waiting for the thread. */
- return EINVAL;
-
- /* BLOCK waits either indefinitely or based on an absolute time. POSIX also
- states a cancellation point shall occur for pthread_join, and we use the
- same rationale for posix_timedjoin_np. Both clockwait_tid and the futex
- call use the cancellable variant. */
- if (block)
+ int result = 0;
+ unsigned int state;
+ while ((state = atomic_load_acquire (&pd->joinstate))
+ != THREAD_STATE_EXITED)
{
- /* During the wait we change to asynchronous cancellation. If we
- are cancelled the thread we are waiting for must be marked as
- un-wait-ed for again. */
- pthread_cleanup_push (cleanup, &pd->joinid);
-
- /* We need acquire MO here so that we synchronize with the
- kernel's store to 0 when the clone terminates. (see above) */
- pid_t tid;
- while ((tid = atomic_load_acquire (&pd->tid)) != 0)
- {
- /* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via
- futex wake-up when the clone terminates. The memory location
- contains the thread ID while the clone is running and is reset to
- zero by the kernel afterwards. The kernel up to version 3.16.3
- does not use the private futex operations for futex wake-up when
- the clone terminates. */
- int ret = __futex_abstimed_wait_cancelable64 (
- (unsigned int *) &pd->tid, tid, clockid, abstime, LLL_SHARED);
- if (ret == ETIMEDOUT || ret == EOVERFLOW)
- {
- result = ret;
- break;
- }
+ if (check_for_deadlock (state, pd))
+ return EDEADLK;
+
+ /* POSIX states calling pthread_join() on a non joinable thread is
+ undefined. However, if PD is still in the cache we can still warn
+ the caller. */
+ if (state == THREAD_STATE_DETACHED)
+ return EINVAL;
+
+ /* pthread_join() is a cancellation entrypoint and we use the same
+ rationale for pthread_timedjoin_np().
+
+ The kernel notifies a process which uses CLONE_CHILD_CLEARTID via
+ a memory zerouing and futex wake up when the process terminates.
+ The futex operation is not private. */
+ int ret = __futex_abstimed_wait_cancelable64 (&pd->joinstate, state,
+ clockid, abstime,
+ LLL_SHARED);
+ if (ret == ETIMEDOUT || ret == EOVERFLOW)
+ {
+ result = ret;
+ break;
}
-
- pthread_cleanup_pop (0);
}
void *pd_result = pd->result;
- if (__glibc_likely (result == 0))
+ if (result == 0)
{
- /* We mark the thread as terminated and as joined. */
- pd->tid = -1;
-
- /* Store the return value if the caller is interested. */
+ pd->tid = 0;
if (thread_return != NULL)
*thread_return = pd_result;
-
- /* Free the TCB. */
__nptl_free_tcb (pd);
}
- else
- pd->joinid = NULL;
LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd_result);
diff --git a/nptl/pthread_timedjoin.c b/nptl/pthread_timedjoin.c
index ebc31f935a..b35c8f4279 100644
--- a/nptl/pthread_timedjoin.c
+++ b/nptl/pthread_timedjoin.c
@@ -25,7 +25,7 @@ ___pthread_timedjoin_np64 (pthread_t threadid, void **thread_return,
const struct __timespec64 *abstime)
{
return __pthread_clockjoin_ex (threadid, thread_return,
- CLOCK_REALTIME, abstime, true);
+ CLOCK_REALTIME, abstime);
}
#if __TIMESIZE == 64
diff --git a/nptl/pthread_tryjoin.c b/nptl/pthread_tryjoin.c
index fd938e8780..726f2abc68 100644
--- a/nptl/pthread_tryjoin.c
+++ b/nptl/pthread_tryjoin.c
@@ -22,15 +22,17 @@
int
__pthread_tryjoin_np (pthread_t threadid, void **thread_return)
{
- /* Return right away if the thread hasn't terminated yet. */
- struct pthread *pd = (struct pthread *) threadid;
- if (pd->tid != 0)
- return EBUSY;
+ /* The joinable state (THREAD_STATE_JOINABLE) is straigthforward since the
+ thread hasn't finished yet and trying to join might block.
+ Both detached (THREAD_STATE_DETACHED) and exiting (THREAD_STATE_EXITING)
+ might also result in a possible blocking call: a detached thread might
+ change its state to exiting and a exiting thread my take some time to
+ exit (and thus let the kernel set the state to THREAD_STATE_EXITED). */
- /* If pd->tid == 0 then lll_wait_tid will not block on futex
- operation. */
- return __pthread_clockjoin_ex (threadid, thread_return, 0 /* Ignored */,
- NULL, false);
+ struct pthread *pd = (struct pthread *) threadid;
+ return atomic_load_acquire (&pd->joinstate) != THREAD_STATE_EXITED
+ ? EBUSY
+ : __pthread_clockjoin_ex (threadid, thread_return, 0, NULL);
}
versioned_symbol (libc, __pthread_tryjoin_np, pthread_tryjoin_np, GLIBC_2_34);
diff --git a/sysdeps/nptl/dl-tls_init_tp.c b/sysdeps/nptl/dl-tls_init_tp.c
index b7b3bb1cdb..32a9acd8f7 100644
--- a/sysdeps/nptl/dl-tls_init_tp.c
+++ b/sysdeps/nptl/dl-tls_init_tp.c
@@ -62,7 +62,7 @@ __tls_init_tp (void)
/* Early initialization of the TCB. */
struct pthread *pd = THREAD_SELF;
- pd->tid = INTERNAL_SYSCALL_CALL (set_tid_address, &pd->tid);
+ pd->tid = INTERNAL_SYSCALL_CALL (set_tid_address, &pd->joinstate);
THREAD_SETMEM (pd, specific[0], &pd->specific_1stblock[0]);
THREAD_SETMEM (pd, user_stack, true);
@@ -97,4 +97,6 @@ __tls_init_tp (void)
THREAD_SETMEM (pd, cancelstate, PTHREAD_CANCEL_ENABLE);
THREAD_SETMEM (pd, canceltype, PTHREAD_CANCEL_DEFERRED);
+
+ THREAD_SETMEM (pd, joinstate, THREAD_STATE_JOINABLE);
}
diff --git a/sysdeps/nptl/libc_start_call_main.h b/sysdeps/nptl/libc_start_call_main.h
index b56bf34325..592326e977 100644
--- a/sysdeps/nptl/libc_start_call_main.h
+++ b/sysdeps/nptl/libc_start_call_main.h
@@ -17,6 +17,7 @@
<https://www.gnu.org/licenses/>. */
#include <atomic.h>
+#include <futex-internal.h>
#include <nptl/pthreadP.h>
_Noreturn static void
@@ -65,6 +66,12 @@ __libc_start_call_main (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
/* One less thread. Decrement the counter. If it is zero we
terminate the entire process. */
result = 0;
+
+ /* For the case a thread is waiting for the main thread to finish. */
+ struct pthread *self = THREAD_SELF;
+ atomic_store_release (&self->joinstate, THREAD_STATE_EXITED);
+ futex_wake (&self->joinstate, 1, FUTEX_SHARED);
+
if (! atomic_decrement_and_test (&__nptl_nthreads))
/* Not much left to do but to exit the thread, not the process. */
while (1)
diff --git a/sysdeps/pthread/tst-thrd-detach.c b/sysdeps/pthread/tst-thrd-detach.c
index c844767748..e1906a0e10 100644
--- a/sysdeps/pthread/tst-thrd-detach.c
+++ b/sysdeps/pthread/tst-thrd-detach.c
@@ -20,14 +20,14 @@
#include <time.h>
#include <stdio.h>
#include <unistd.h>
-
+#include <limits.h>
#include <support/check.h>
static int
detach_thrd (void *arg)
{
- if (thrd_detach (thrd_current ()) != thrd_success)
- FAIL_EXIT1 ("thrd_detach failed");
+ thrd_sleep (&(struct timespec) { .tv_sec = INT_MAX }, NULL);
+
thrd_exit (thrd_success);
}
@@ -36,15 +36,11 @@ do_test (void)
{
thrd_t id;
- /* Create new thread. */
- if (thrd_create (&id, detach_thrd, NULL) != thrd_success)
- FAIL_EXIT1 ("thrd_create failed");
+ TEST_COMPARE (thrd_create (&id, detach_thrd, NULL), thrd_success);
- /* Give some time so the thread can finish. */
- thrd_sleep (&(struct timespec) {.tv_sec = 2}, NULL);
+ TEST_COMPARE (thrd_detach (id), thrd_success);
- if (thrd_join (id, NULL) == thrd_success)
- FAIL_EXIT1 ("thrd_join succeed where it should fail");
+ TEST_COMPARE (thrd_join (id, NULL), thrd_error);
return 0;
}
--
2.30.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 5/6] nptl: Move setxid flag out of cancelhandling
2021-06-10 19:36 [PATCH 0/6] More NPTL fixes Adhemerval Zanella
` (3 preceding siblings ...)
2021-06-10 19:36 ` [PATCH 4/6] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951) Adhemerval Zanella
@ 2021-06-10 19:36 ` Adhemerval Zanella
2021-06-10 19:36 ` [PATCH 6/6] nptl: Replace struct thread cancelhandling field Adhemerval Zanella
5 siblings, 0 replies; 11+ messages in thread
From: Adhemerval Zanella @ 2021-06-10 19:36 UTC (permalink / raw)
To: libc-alpha
Now that the thread state is tracked by PD->joinstate, there is no need
to keep the setxid flag within the PD->cancelhandling. It simplifies
the atomic code to set and reset it (since there is no need to handle
the thread state concurrent update).
Checked on x86_64-linux-gnu and aarch64-linux-gnu.
---
nptl/allocatestack.c | 3 +++
nptl/descr.h | 5 ++---
nptl/nptl_setxid.c | 49 ++++++++++---------------------------------
nptl/pthread_create.c | 4 ++--
4 files changed, 18 insertions(+), 43 deletions(-)
diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index 9be6c42894..aae2281517 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -140,6 +140,7 @@ get_cached_stack (size_t *sizep, void **memp)
}
/* Don't allow setxid until cloned. */
+ result->setxid_flag = 0;
result->setxid_futex = -1;
/* Dequeue the entry. */
@@ -342,6 +343,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
#endif
/* Don't allow setxid until cloned. */
+ pd->setxid_flag = 0;
pd->setxid_futex = -1;
/* Allocate the DTV for this thread. */
@@ -463,6 +465,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
#endif
/* Don't allow setxid until cloned. */
+ pd->setxid_flag = 0;
pd->setxid_futex = -1;
/* Allocate the DTV for this thread. */
diff --git a/nptl/descr.h b/nptl/descr.h
index 4b2db6edab..563b152bff 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -297,9 +297,6 @@ struct pthread
/* Bit set if thread terminated and TCB is freed. */
#define TERMINATED_BIT 5
#define TERMINATED_BITMASK (0x01 << TERMINATED_BIT)
- /* Bit set if thread is supposed to change XID. */
-#define SETXID_BIT 6
-#define SETXID_BITMASK (0x01 << SETXID_BIT)
/* Flags. Including those copied from the thread attribute. */
int flags;
@@ -339,6 +336,8 @@ struct pthread
/* Lock to synchronize access to the descriptor. */
int lock;
+ /* Indicate whether thread is supposed to change XID. */
+ int setxid_flag;
/* Lock for synchronizing setxid calls. */
unsigned int setxid_futex;
diff --git a/nptl/nptl_setxid.c b/nptl/nptl_setxid.c
index 2f35772411..c337fa8577 100644
--- a/nptl/nptl_setxid.c
+++ b/nptl/nptl_setxid.c
@@ -76,14 +76,7 @@ __nptl_setxid_sighandler (int sig, siginfo_t *si, void *ctx)
/* Reset the SETXID flag. */
struct pthread *self = THREAD_SELF;
- int flags, newval;
- do
- {
- flags = THREAD_GETMEM (self, cancelhandling);
- newval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling,
- flags & ~SETXID_BITMASK, flags);
- }
- while (flags != newval);
+ atomic_store_release (&self->setxid_flag, 0);
/* And release the futex. */
self->setxid_futex = 1;
@@ -97,8 +90,6 @@ libc_hidden_def (__nptl_setxid_sighandler)
static void
setxid_mark_thread (struct xid_command *cmdp, struct pthread *t)
{
- int ch;
-
/* Wait until this thread is cloned. */
if (t->setxid_futex == -1
&& ! atomic_compare_and_exchange_bool_acq (&t->setxid_futex, -2, -1))
@@ -109,41 +100,23 @@ setxid_mark_thread (struct xid_command *cmdp, struct pthread *t)
/* Don't let the thread exit before the setxid handler runs. */
t->setxid_futex = 0;
- do
- {
- ch = t->cancelhandling;
+ /* If thread is exiting right now, ignore it. */
+ if (atomic_load_acquire (&t->joinstate) == THREAD_STATE_EXITING)
+ return;
- /* If the thread is exiting right now, ignore it. */
- if ((ch & EXITING_BITMASK) != 0)
- {
- /* Release the futex if there is no other setxid in
- progress. */
- if ((ch & SETXID_BITMASK) == 0)
- {
- t->setxid_futex = 1;
- futex_wake (&t->setxid_futex, 1, FUTEX_PRIVATE);
- }
- return;
- }
+ /* Release the futex if there is no other setxid in progress. */
+ if (atomic_exchange_acquire (&t->setxid_flag, 1) == 0)
+ {
+ t->setxid_futex = 1;
+ futex_wake (&t->setxid_futex, 1, FUTEX_PRIVATE);
}
- while (atomic_compare_and_exchange_bool_acq (&t->cancelhandling,
- ch | SETXID_BITMASK, ch));
}
static void
setxid_unmark_thread (struct xid_command *cmdp, struct pthread *t)
{
- int ch;
-
- do
- {
- ch = t->cancelhandling;
- if ((ch & SETXID_BITMASK) == 0)
- return;
- }
- while (atomic_compare_and_exchange_bool_acq (&t->cancelhandling,
- ch & ~SETXID_BITMASK, ch));
+ atomic_exchange_release (&t->setxid_flag, 0);
/* Release the futex just in case. */
t->setxid_futex = 1;
@@ -154,7 +127,7 @@ setxid_unmark_thread (struct xid_command *cmdp, struct pthread *t)
static int
setxid_signal_thread (struct xid_command *cmdp, struct pthread *t)
{
- if ((t->cancelhandling & SETXID_BITMASK) == 0)
+ if (atomic_load_relaxed (&t->setxid_flag) == 0)
return 0;
int val;
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index 0c0f2c03e5..0b1c67f6d2 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -531,7 +531,7 @@ start_thread (void *arg)
advise_stack_range (pd->stackblock, pd->stackblock_size, (uintptr_t) pd,
pd->guardsize);
- if (__glibc_unlikely (pd->cancelhandling & SETXID_BITMASK))
+ if (__glibc_unlikely (atomic_load_relaxed (&pd->setxid_flag) == 1))
{
/* Some other thread might call any of the setXid functions and expect
us to reply. In this case wait until we did that. */
@@ -541,7 +541,7 @@ start_thread (void *arg)
condition used in the surrounding loop (cancelhandling). We need
to check and document why this is correct. */
futex_wait_simple (&pd->setxid_futex, 0, FUTEX_PRIVATE);
- while (pd->cancelhandling & SETXID_BITMASK);
+ while (atomic_load_relaxed (&pd->setxid_flag) == 1);
/* Reset the value so that the stack can be reused. */
pd->setxid_futex = 0;
--
2.30.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 6/6] nptl: Replace struct thread cancelhandling field
2021-06-10 19:36 [PATCH 0/6] More NPTL fixes Adhemerval Zanella
` (4 preceding siblings ...)
2021-06-10 19:36 ` [PATCH 5/6] nptl: Move setxid flag out of cancelhandling Adhemerval Zanella
@ 2021-06-10 19:36 ` Adhemerval Zanella
5 siblings, 0 replies; 11+ messages in thread
From: Adhemerval Zanella @ 2021-06-10 19:36 UTC (permalink / raw)
To: libc-alpha
Now that both thread state and setxid is being tracked by two different
PD members (joinstate and setxid_flag), there is no need to keep track
of exiting (EXITING_BIT) and terminated (TERMINATED_BIT) state in
'cancelhandling'.
The 'cancelhandling' field is renamed to 'cancelstatus' and now only
signal whether the thread has been cancelled (set by pthread_cancel()).
It allows simplify the atomic operation required.
There is no need to check the thread state on
__pthread_enable_asynccancel() nor on pthread_testcancel () anymore now
that cancellation is explicit disabled when thread start the exit code
(__do_cancel()).
On __nptl_free_tcp(), the PD->joinstate now defines whether is
the creating thread or created thread that calls it. So there is
no concurrent call of the function and thus no need to set the
TERMINATED_BIT.
For SIGCANCEL handler, sigcancel_handler(), PD->joinstate is used
instead (pthread_cancel() might still be called concurrenty in
assynchronous mode).
Finally the nptl_db is adjusted to check the PD->joinstate information
instead of cancelhandling to check for the thread state.
Checked on x86_64-linux-gnu and aarch64-linux-gnu.
---
nptl/allocatestack.c | 2 +-
nptl/cancellation.c | 10 ++--------
nptl/descr.h | 13 ++-----------
nptl/nptl_free_tcb.c | 22 ++++++----------------
nptl/pthreadP.h | 3 ---
nptl/pthread_cancel.c | 10 ++++------
nptl/pthread_create.c | 10 ++--------
nptl/pthread_join_common.c | 10 +++-------
nptl/pthread_testcancel.c | 10 ++--------
nptl_db/structs.def | 2 +-
nptl_db/td_thr_get_info.c | 16 +++++++---------
nptl_db/td_thr_getfpregs.c | 9 +++++----
nptl_db/td_thr_getgregs.c | 9 +++++----
nptl_db/td_thr_setfpregs.c | 9 +++++----
nptl_db/td_thr_setgregs.c | 9 +++++----
sysdeps/hppa/nptl/tcb-offsets.sym | 1 -
sysdeps/i386/nptl/tcb-offsets.sym | 1 -
sysdeps/sh/nptl/tcb-offsets.sym | 1 -
sysdeps/x86_64/nptl/tcb-offsets.sym | 4 ----
19 files changed, 50 insertions(+), 101 deletions(-)
diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index aae2281517..f9829e2dae 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -160,7 +160,7 @@ get_cached_stack (size_t *sizep, void **memp)
*memp = result->stackblock;
/* Cancellation handling is back to the default. */
- result->cancelhandling = 0;
+ result->cancelstatus = 0;
result->cancelstate = PTHREAD_CANCEL_ENABLE;
result->canceltype = PTHREAD_CANCEL_DEFERRED;
result->cleanup = NULL;
diff --git a/nptl/cancellation.c b/nptl/cancellation.c
index 6478c029de..a244b3eeea 100644
--- a/nptl/cancellation.c
+++ b/nptl/cancellation.c
@@ -35,15 +35,9 @@ __pthread_enable_asynccancel (void)
int oldval = THREAD_GETMEM (self, canceltype);
THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_ASYNCHRONOUS);
- int ch = THREAD_GETMEM (self, cancelhandling);
-
if (self->cancelstate == PTHREAD_CANCEL_ENABLE
- && (ch & CANCELED_BITMASK)
- && !(ch & EXITING_BITMASK)
- && !(ch & TERMINATED_BITMASK))
- {
- __do_cancel ();
- }
+ && self->cancelstatus == 1)
+ __do_cancel ();
return oldval;
}
diff --git a/nptl/descr.h b/nptl/descr.h
index 563b152bff..1bfa2b9b52 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -286,17 +286,8 @@ struct pthread
struct pthread_unwind_buf *cleanup_jmp_buf;
#define HAVE_CLEANUP_JMP_BUF
- /* Flags determining processing of cancellation. */
- int cancelhandling;
- /* Bit set if canceled. */
-#define CANCELED_BIT 3
-#define CANCELED_BITMASK (0x01 << CANCELED_BIT)
- /* Bit set if thread is exiting. */
-#define EXITING_BIT 4
-#define EXITING_BITMASK (0x01 << EXITING_BIT)
- /* Bit set if thread terminated and TCB is freed. */
-#define TERMINATED_BIT 5
-#define TERMINATED_BITMASK (0x01 << TERMINATED_BIT)
+ /* Flag to determine whether the thread is signaled to be cancelled. */
+ int cancelstatus;
/* Flags. Including those copied from the thread attribute. */
int flags;
diff --git a/nptl/nptl_free_tcb.c b/nptl/nptl_free_tcb.c
index cbf3580f59..15e1a18562 100644
--- a/nptl/nptl_free_tcb.c
+++ b/nptl/nptl_free_tcb.c
@@ -24,22 +24,12 @@
void
__nptl_free_tcb (struct pthread *pd)
{
- /* The thread is exiting now. */
- if (atomic_bit_test_set (&pd->cancelhandling, TERMINATED_BIT) == 0)
- {
- /* Free TPP data. */
- if (pd->tpp != NULL)
- {
- struct priority_protection_data *tpp = pd->tpp;
+ free (pd->tpp);
+ pd->tpp = NULL;
- pd->tpp = NULL;
- free (tpp);
- }
-
- /* Queue the stack memory block for reuse and exit the process. The
- kernel will signal via writing to the address returned by
- QUEUE-STACK when the stack is available. */
- __nptl_deallocate_stack (pd);
- }
+ /* Queue the stack memory block for reuse and exit the process. The kernel
+ will signal via writing to the address returned by QUEUE-STACK when the
+ stack is available. */
+ __nptl_deallocate_stack (pd);
}
libc_hidden_def (__nptl_free_tcb)
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index bfb0e40b44..18b7cfbd79 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -273,9 +273,6 @@ __exit_thread (void *value)
{
struct pthread *self = THREAD_SELF;
- /* Make sure we get no more cancellations. */
- THREAD_ATOMIC_BIT_SET (self, cancelhandling, EXITING_BIT);
-
THREAD_SETMEM (self, result, value);
/* It is required by POSIX XSH 2.9.5 Thread Cancellation under the heading
diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
index 577ff6c746..c3d07258b6 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -43,11 +43,8 @@ sigcancel_handler (int sig, siginfo_t *si, void *ctx)
struct pthread *self = THREAD_SELF;
- int ch = atomic_load_relaxed (&self->cancelhandling);
- /* Cancelation not enabled, not cancelled, or already exitting. */
if (self->cancelstate == PTHREAD_CANCEL_DISABLE
- || (ch & CANCELED_BITMASK) == 0
- || (ch & EXITING_BITMASK) != 0)
+ || atomic_load_relaxed (&self->joinstate) == THREAD_STATE_EXITING)
return;
/* Set the return value. */
@@ -88,8 +85,9 @@ __pthread_cancel (pthread_t th)
}
#endif
- int oldch = atomic_fetch_or_acquire (&pd->cancelhandling, CANCELED_BITMASK);
- if ((oldch & CANCELED_BITMASK) != 0)
+ /* If already cancelled just return (cancellation will be acted upon in next
+ cancellation entrypoint). */
+ if (atomic_exchange_acquire (&pd->cancelstatus, 1) == 1)
return 0;
if (pd == THREAD_SELF)
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index 0b1c67f6d2..f34fbff096 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -469,12 +469,6 @@ start_thread (void *arg)
}
}
- /* The thread is exiting now. Don't set this bit until after we've hit
- the event-reporting breakpoint, so that td_thr_get_info on us while at
- the breakpoint reports TD_THR_RUN state rather than TD_THR_ZOMBIE. */
- atomic_bit_set (&pd->cancelhandling, EXITING_BIT);
-
-
/* CONCURRENCY NOTES:
Concurrent pthread_detach() will either set state to
@@ -538,8 +532,8 @@ start_thread (void *arg)
do
/* XXX This differs from the typical futex_wait_simple pattern in that
the futex_wait condition (setxid_futex) is different from the
- condition used in the surrounding loop (cancelhandling). We need
- to check and document why this is correct. */
+ condition used in the surrounding loop. We need to check and
+ document why this is correct. */
futex_wait_simple (&pd->setxid_futex, 0, FUTEX_PRIVATE);
while (atomic_load_relaxed (&pd->setxid_flag) == 1);
diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
index 0870f36ecb..a131efba3b 100644
--- a/nptl/pthread_join_common.c
+++ b/nptl/pthread_join_common.c
@@ -33,14 +33,10 @@ check_for_deadlock (int state, struct pthread *pd)
{
struct pthread *self = THREAD_SELF;
return ((pd == self
- || (atomic_load_acquire (&self->joinstate) == THREAD_STATE_DETACHED
- && (pd->cancelhandling
- & (CANCELED_BITMASK | EXITING_BITMASK
- | TERMINATED_BITMASK)) == 0))
+ || (atomic_load_acquire (&self->joinstate)
+ == THREAD_STATE_DETACHED))
&& !(self->cancelstate == PTHREAD_CANCEL_ENABLE
- && (pd->cancelhandling & (CANCELED_BITMASK | EXITING_BITMASK
- | TERMINATED_BITMASK))
- == CANCELED_BITMASK));
+ && atomic_load_relaxed (&self->cancelstatus) == 1));
}
int
diff --git a/nptl/pthread_testcancel.c b/nptl/pthread_testcancel.c
index e6adf12580..b06327ebe3 100644
--- a/nptl/pthread_testcancel.c
+++ b/nptl/pthread_testcancel.c
@@ -24,14 +24,8 @@ void
___pthread_testcancel (void)
{
struct pthread *self = THREAD_SELF;
- int cancelhandling = THREAD_GETMEM (self, cancelhandling);
- if (self->cancelstate == PTHREAD_CANCEL_ENABLE
- && (cancelhandling & CANCELED_BITMASK)
- && !(cancelhandling & EXITING_BITMASK)
- && !(cancelhandling & TERMINATED_BITMASK))
- {
- __do_cancel ();
- }
+ if (self->cancelstate == PTHREAD_CANCEL_ENABLE && self->cancelstatus == 1)
+ __do_cancel ();
}
versioned_symbol (libc, ___pthread_testcancel, pthread_testcancel, GLIBC_2_34);
versioned_symbol (libc, ___pthread_testcancel, __pthread_testcancel,
diff --git a/nptl_db/structs.def b/nptl_db/structs.def
index 6a726f207e..26f9a41f1a 100644
--- a/nptl_db/structs.def
+++ b/nptl_db/structs.def
@@ -57,7 +57,7 @@ DB_STRUCT_FIELD (pthread, list)
DB_STRUCT_FIELD (pthread, report_events)
DB_STRUCT_FIELD (pthread, tid)
DB_STRUCT_FIELD (pthread, start_routine)
-DB_STRUCT_FIELD (pthread, cancelhandling)
+DB_STRUCT_FIELD (pthread, joinstate)
DB_STRUCT_FIELD (pthread, schedpolicy)
DB_STRUCT_FIELD (pthread, schedparam_sched_priority)
DB_STRUCT_FIELD (pthread, specific)
diff --git a/nptl_db/td_thr_get_info.c b/nptl_db/td_thr_get_info.c
index 01af021d2a..82f1f2667c 100644
--- a/nptl_db/td_thr_get_info.c
+++ b/nptl_db/td_thr_get_info.c
@@ -28,7 +28,7 @@ td_thr_get_info (const td_thrhandle_t *th, td_thrinfo_t *infop)
{
td_err_e err;
void *copy;
- psaddr_t tls, schedpolicy, schedprio, cancelhandling, tid, report_events;
+ psaddr_t tls, schedpolicy, schedprio, joinstate, tid, report_events;
LOG ("td_thr_get_info");
@@ -37,7 +37,7 @@ td_thr_get_info (const td_thrhandle_t *th, td_thrinfo_t *infop)
/* Special case for the main thread before initialization. */
copy = NULL;
tls = 0;
- cancelhandling = 0;
+ joinstate = 0;
schedpolicy = SCHED_OTHER;
schedprio = 0;
tid = 0;
@@ -76,8 +76,8 @@ td_thr_get_info (const td_thrhandle_t *th, td_thrinfo_t *infop)
err = DB_GET_FIELD_LOCAL (tid, th->th_ta_p, copy, pthread, tid, 0);
if (err != TD_OK)
return err;
- err = DB_GET_FIELD_LOCAL (cancelhandling, th->th_ta_p, copy, pthread,
- cancelhandling, 0);
+ err = DB_GET_FIELD_LOCAL (joinstate, th->th_ta_p, copy, pthread,
+ joinstate, 0);
if (err != TD_OK)
return err;
err = DB_GET_FIELD_LOCAL (report_events, th->th_ta_p, copy, pthread,
@@ -96,13 +96,11 @@ td_thr_get_info (const td_thrhandle_t *th, td_thrinfo_t *infop)
? 0 : (uintptr_t) schedprio);
infop->ti_type = TD_THR_USER;
- if ((((int) (uintptr_t) cancelhandling) & EXITING_BITMASK) == 0)
- /* XXX For now there is no way to get more information. */
+ int js = (int) (uintptr_t) joinstate;
+ if (js == THREAD_STATE_JOINABLE || js == THREAD_STATE_DETACHED)
infop->ti_state = TD_THR_ACTIVE;
- else if ((((int) (uintptr_t) cancelhandling) & TERMINATED_BITMASK) == 0)
- infop->ti_state = TD_THR_ZOMBIE;
else
- infop->ti_state = TD_THR_UNKNOWN;
+ infop->ti_state = TD_THR_ZOMBIE;
/* Initialization which are the same in both cases. */
infop->ti_ta_p = th->th_ta_p;
diff --git a/nptl_db/td_thr_getfpregs.c b/nptl_db/td_thr_getfpregs.c
index 3d08aa3f60..23a8a215c2 100644
--- a/nptl_db/td_thr_getfpregs.c
+++ b/nptl_db/td_thr_getfpregs.c
@@ -23,7 +23,7 @@
td_err_e
td_thr_getfpregs (const td_thrhandle_t *th, prfpregset_t *regset)
{
- psaddr_t cancelhandling, tid;
+ psaddr_t joinstate, tid;
td_err_e err;
LOG ("td_thr_getfpregs");
@@ -34,13 +34,14 @@ td_thr_getfpregs (const td_thrhandle_t *th, prfpregset_t *regset)
regset) != PS_OK ? TD_ERR : TD_OK;
/* We have to get the state and the PID for this thread. */
- err = DB_GET_FIELD (cancelhandling, th->th_ta_p, th->th_unique, pthread,
- cancelhandling, 0);
+ err = DB_GET_FIELD (joinstate, th->th_ta_p, th->th_unique, pthread,
+ joinstate, 0);
if (err != TD_OK)
return err;
/* If the thread already terminated we return all zeroes. */
- if (((int) (uintptr_t) cancelhandling) & TERMINATED_BITMASK)
+ int js = (int) (uintptr_t) joinstate;
+ if (js == THREAD_STATE_EXITING || js == THREAD_STATE_EXITED)
memset (regset, '\0', sizeof (*regset));
/* Otherwise get the register content through the callback. */
else
diff --git a/nptl_db/td_thr_getgregs.c b/nptl_db/td_thr_getgregs.c
index 8f9fab096f..b92402670f 100644
--- a/nptl_db/td_thr_getgregs.c
+++ b/nptl_db/td_thr_getgregs.c
@@ -23,7 +23,7 @@
td_err_e
td_thr_getgregs (const td_thrhandle_t *th, prgregset_t regset)
{
- psaddr_t cancelhandling, tid;
+ psaddr_t joinstate, tid;
td_err_e err;
LOG ("td_thr_getgregs");
@@ -34,13 +34,14 @@ td_thr_getgregs (const td_thrhandle_t *th, prgregset_t regset)
regset) != PS_OK ? TD_ERR : TD_OK;
/* We have to get the state and the PID for this thread. */
- err = DB_GET_FIELD (cancelhandling, th->th_ta_p, th->th_unique, pthread,
- cancelhandling, 0);
+ err = DB_GET_FIELD (joinstate, th->th_ta_p, th->th_unique, pthread,
+ joinstate, 0);
if (err != TD_OK)
return err;
/* If the thread already terminated we return all zeroes. */
- if (((int) (uintptr_t) cancelhandling) & TERMINATED_BITMASK)
+ int js = (int) (uintptr_t) joinstate;
+ if (js == THREAD_STATE_EXITING || js == THREAD_STATE_EXITED)
memset (regset, '\0', sizeof (*regset));
/* Otherwise get the register content through the callback. */
else
diff --git a/nptl_db/td_thr_setfpregs.c b/nptl_db/td_thr_setfpregs.c
index bddb0359a8..73ab675ce6 100644
--- a/nptl_db/td_thr_setfpregs.c
+++ b/nptl_db/td_thr_setfpregs.c
@@ -23,7 +23,7 @@
td_err_e
td_thr_setfpregs (const td_thrhandle_t *th, const prfpregset_t *fpregs)
{
- psaddr_t cancelhandling, tid;
+ psaddr_t joinstate, tid;
td_err_e err;
LOG ("td_thr_setfpregs");
@@ -34,13 +34,14 @@ td_thr_setfpregs (const td_thrhandle_t *th, const prfpregset_t *fpregs)
fpregs) != PS_OK ? TD_ERR : TD_OK;
/* We have to get the state and the PID for this thread. */
- err = DB_GET_FIELD (cancelhandling, th->th_ta_p, th->th_unique, pthread,
- cancelhandling, 0);
+ err = DB_GET_FIELD (joinstate, th->th_ta_p, th->th_unique, pthread,
+ joinstate, 0);
if (err != TD_OK)
return err;
/* Only set the registers if the thread hasn't yet terminated. */
- if ((((int) (uintptr_t) cancelhandling) & TERMINATED_BITMASK) == 0)
+ int js = (int) (uintptr_t) joinstate;
+ if (js != THREAD_STATE_EXITING || js != THREAD_STATE_EXITED)
{
err = DB_GET_FIELD (tid, th->th_ta_p, th->th_unique, pthread, tid, 0);
if (err != TD_OK)
diff --git a/nptl_db/td_thr_setgregs.c b/nptl_db/td_thr_setgregs.c
index 2a76a10754..186df94cbc 100644
--- a/nptl_db/td_thr_setgregs.c
+++ b/nptl_db/td_thr_setgregs.c
@@ -23,7 +23,7 @@
td_err_e
td_thr_setgregs (const td_thrhandle_t *th, prgregset_t gregs)
{
- psaddr_t cancelhandling, tid;
+ psaddr_t joinstate, tid;
td_err_e err;
LOG ("td_thr_setgregs");
@@ -34,13 +34,14 @@ td_thr_setgregs (const td_thrhandle_t *th, prgregset_t gregs)
gregs) != PS_OK ? TD_ERR : TD_OK;
/* We have to get the state and the PID for this thread. */
- err = DB_GET_FIELD (cancelhandling, th->th_ta_p, th->th_unique, pthread,
- cancelhandling, 0);
+ err = DB_GET_FIELD (joinstate, th->th_ta_p, th->th_unique, pthread,
+ joinstate, 0);
if (err != TD_OK)
return err;
/* Only set the registers if the thread hasn't yet terminated. */
- if ((((int) (uintptr_t) cancelhandling) & TERMINATED_BITMASK) == 0)
+ int js = (int) (uintptr_t) joinstate;
+ if (js != THREAD_STATE_EXITING || js != THREAD_STATE_EXITED)
{
err = DB_GET_FIELD (tid, th->th_ta_p, th->th_unique, pthread, tid, 0);
if (err != TD_OK)
diff --git a/sysdeps/hppa/nptl/tcb-offsets.sym b/sysdeps/hppa/nptl/tcb-offsets.sym
index 6e852f35b1..8937f1ec21 100644
--- a/sysdeps/hppa/nptl/tcb-offsets.sym
+++ b/sysdeps/hppa/nptl/tcb-offsets.sym
@@ -3,7 +3,6 @@
RESULT offsetof (struct pthread, result)
TID offsetof (struct pthread, tid)
-CANCELHANDLING offsetof (struct pthread, cancelhandling)
CLEANUP_JMP_BUF offsetof (struct pthread, cleanup_jmp_buf)
MULTIPLE_THREADS_OFFSET offsetof (struct pthread, header.multiple_threads)
TLS_PRE_TCB_SIZE sizeof (struct pthread)
diff --git a/sysdeps/i386/nptl/tcb-offsets.sym b/sysdeps/i386/nptl/tcb-offsets.sym
index 2ec9e787c1..85a27dc29f 100644
--- a/sysdeps/i386/nptl/tcb-offsets.sym
+++ b/sysdeps/i386/nptl/tcb-offsets.sym
@@ -4,7 +4,6 @@
RESULT offsetof (struct pthread, result)
TID offsetof (struct pthread, tid)
-CANCELHANDLING offsetof (struct pthread, cancelhandling)
CLEANUP_JMP_BUF offsetof (struct pthread, cleanup_jmp_buf)
MULTIPLE_THREADS_OFFSET offsetof (tcbhead_t, multiple_threads)
SYSINFO_OFFSET offsetof (tcbhead_t, sysinfo)
diff --git a/sysdeps/sh/nptl/tcb-offsets.sym b/sysdeps/sh/nptl/tcb-offsets.sym
index 234207779d..60c9e40b72 100644
--- a/sysdeps/sh/nptl/tcb-offsets.sym
+++ b/sysdeps/sh/nptl/tcb-offsets.sym
@@ -4,7 +4,6 @@
RESULT offsetof (struct pthread, result)
TID offsetof (struct pthread, tid)
-CANCELHANDLING offsetof (struct pthread, cancelhandling)
CLEANUP_JMP_BUF offsetof (struct pthread, cleanup_jmp_buf)
MULTIPLE_THREADS_OFFSET offsetof (struct pthread, header.multiple_threads)
TLS_PRE_TCB_SIZE sizeof (struct pthread)
diff --git a/sysdeps/x86_64/nptl/tcb-offsets.sym b/sysdeps/x86_64/nptl/tcb-offsets.sym
index 2bbd563a6c..6cc845f7ed 100644
--- a/sysdeps/x86_64/nptl/tcb-offsets.sym
+++ b/sysdeps/x86_64/nptl/tcb-offsets.sym
@@ -4,7 +4,6 @@
RESULT offsetof (struct pthread, result)
TID offsetof (struct pthread, tid)
-CANCELHANDLING offsetof (struct pthread, cancelhandling)
CLEANUP_JMP_BUF offsetof (struct pthread, cleanup_jmp_buf)
CLEANUP offsetof (struct pthread, cleanup)
CLEANUP_PREV offsetof (struct _pthread_cleanup_buffer, __prev)
@@ -13,6 +12,3 @@ MULTIPLE_THREADS_OFFSET offsetof (tcbhead_t, multiple_threads)
POINTER_GUARD offsetof (tcbhead_t, pointer_guard)
FEATURE_1_OFFSET offsetof (tcbhead_t, feature_1)
SSP_BASE_OFFSET offsetof (tcbhead_t, ssp_base)
-
--- Not strictly offsets, but these values are also used in the TCB.
-TCB_CANCELED_BITMASK CANCELED_BITMASK
--
2.30.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/6] nptl: Fix tst-cancel7 and tst-cancelx7 race condition (BZ #14232)
2021-06-10 19:36 ` [PATCH 1/6] nptl: Fix tst-cancel7 and tst-cancelx7 race condition (BZ #14232) Adhemerval Zanella
@ 2021-06-11 15:02 ` Florian Weimer
2021-06-11 15:08 ` Adhemerval Zanella
0 siblings, 1 reply; 11+ messages in thread
From: Florian Weimer @ 2021-06-11 15:02 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella via Libc-alpha:
> +static const char sem_name[] = "/tst-cancel7-sem";
I don't think this is valid, it will break with concurrent execution of
the test suite from different build trees.
Maybe you could use support_shared_allocate and sem_init with a
process-shared semaphore?
Thanks,
Florian
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/6] nptl: Fix tst-cancel7 and tst-cancelx7 race condition (BZ #14232)
2021-06-11 15:02 ` Florian Weimer
@ 2021-06-11 15:08 ` Adhemerval Zanella
2021-06-11 15:10 ` Florian Weimer
0 siblings, 1 reply; 11+ messages in thread
From: Adhemerval Zanella @ 2021-06-11 15:08 UTC (permalink / raw)
To: Florian Weimer, Adhemerval Zanella via Libc-alpha
On 11/06/2021 12:02, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> +static const char sem_name[] = "/tst-cancel7-sem";
>
> I don't think this is valid, it will break with concurrent execution of
> the test suite from different build trees.
>
> Maybe you could use support_shared_allocate and sem_init with a
> process-shared semaphore?
Indeed I did not take this in consideration. But support_shared_allocate is
similar to the unnamed semaphore and the test will use a system() to spawn
the test process. So support_shared_allocate does not really help here.
Maybe a better option would be to move to test-container.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/6] nptl: Fix tst-cancel7 and tst-cancelx7 race condition (BZ #14232)
2021-06-11 15:08 ` Adhemerval Zanella
@ 2021-06-11 15:10 ` Florian Weimer
2021-06-11 17:24 ` [PATCH v2 " Adhemerval Zanella
0 siblings, 1 reply; 11+ messages in thread
From: Florian Weimer @ 2021-06-11 15:10 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella:
> On 11/06/2021 12:02, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>>
>>> +static const char sem_name[] = "/tst-cancel7-sem";
>>
>> I don't think this is valid, it will break with concurrent execution of
>> the test suite from different build trees.
>>
>> Maybe you could use support_shared_allocate and sem_init with a
>> process-shared semaphore?
>
> Indeed I did not take this in consideration. But support_shared_allocate is
> similar to the unnamed semaphore and the test will use a system() to spawn
> the test process. So support_shared_allocate does not really help here.
Hmm. I guess you could use a mapped file created with create_temp_file,
and pass the name on the command line.
Thanks,
Florian
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/6] nptl: Fix tst-cancel7 and tst-cancelx7 race condition (BZ #14232)
2021-06-11 15:10 ` Florian Weimer
@ 2021-06-11 17:24 ` Adhemerval Zanella
0 siblings, 0 replies; 11+ messages in thread
From: Adhemerval Zanella @ 2021-06-11 17:24 UTC (permalink / raw)
To: libc-alpha
A mapped temporary file and a semaphore is used to synchronize the
pid information on the created file, the semaphore is updated once
the file contents is flushed.
Checked on x86_64-linux-gnu.
---
nptl/Makefile | 2 +
nptl/tst-cancel7.c | 114 ++++++++++++++++++++++-----------------------
2 files changed, 59 insertions(+), 57 deletions(-)
diff --git a/nptl/Makefile b/nptl/Makefile
index 3e6cf0c21b..0ad9e24b0f 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -539,6 +539,8 @@ else
librt = $(common-objpfx)rt/librt.a
endif
+$(objpfx)tst-cancel7: $(librt)
+$(objpfx)tst-cancelx7: $(librt)
$(objpfx)tst-cancel17: $(librt)
$(objpfx)tst-cancelx17: $(librt)
diff --git a/nptl/tst-cancel7.c b/nptl/tst-cancel7.c
index 7a1870ac74..e3d037530c 100644
--- a/nptl/tst-cancel7.c
+++ b/nptl/tst-cancel7.c
@@ -18,44 +18,48 @@
#include <errno.h>
#include <fcntl.h>
-#include <pthread.h>
+#include <getopt.h>
#include <signal.h>
-#include <stdio.h>
#include <stdlib.h>
-#include <string.h>
-#include <unistd.h>
-#include <getopt.h>
-
+#include <semaphore.h>
+#include <sys/mman.h>
+
+#include <support/check.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/xstdio.h>
+#include <support/xunistd.h>
#include <support/xthread.h>
-const char *command;
-const char *pidfile;
-char pidfilename[] = "/tmp/tst-cancel7-XXXXXX";
+static const char *command;
+static const char *pidfile;
+static const char *semfile;
+static char *pidfilename;
+static char *semfilename;
+
+static sem_t *sem;
static void *
tf (void *arg)
{
- const char *args = " --direct --pidfile ";
- char *cmd = alloca (strlen (command) + strlen (args)
- + strlen (pidfilename) + 1);
-
- strcpy (stpcpy (stpcpy (cmd, command), args), pidfilename);
+ char *cmd = xasprintf ("%s --direct --sem %s --pidfile %s",
+ command, semfilename, pidfilename);
system (cmd);
/* This call should never return. */
return NULL;
}
-
static void
sl (void)
{
- FILE *f = fopen (pidfile, "w");
- if (f == NULL)
- exit (1);
+ FILE *f = xfopen (pidfile, "w");
fprintf (f, "%lld\n", (long long) getpid ());
fflush (f);
+ if (sem_post (sem) != 0)
+ FAIL_EXIT1 ("sem_post: %m");
+
struct flock fl =
{
.l_type = F_WRLCK,
@@ -64,7 +68,7 @@ sl (void)
.l_len = 1
};
if (fcntl (fileno (f), F_SETLK, &fl) != 0)
- exit (1);
+ FAIL_EXIT1 ("fcntl (F_SETFL): %m");
sigset_t ss;
sigfillset (&ss);
@@ -76,57 +80,57 @@ sl (void)
static void
do_prepare (int argc, char *argv[])
{
+ int semfd;
+ if (semfile == NULL)
+ semfd = create_temp_file ("tst-cancel7.", &semfilename);
+ else
+ semfd = open (semfile, O_RDWR);
+ TEST_VERIFY_EXIT (semfd != -1);
+
+ sem = xmmap (NULL, sizeof (sem_t), PROT_READ | PROT_WRITE, MAP_SHARED,
+ semfd);
+ TEST_VERIFY_EXIT (sem != SEM_FAILED);
+ if (semfile == NULL)
+ {
+ xftruncate (semfd, sizeof (sem_t));
+ TEST_VERIFY_EXIT (sem_init (sem, 1, 0) != -1);
+ }
+
if (command == NULL)
command = argv[0];
if (pidfile)
sl ();
- int fd = mkstemp (pidfilename);
+ int fd = create_temp_file ("tst-cancel7-pid-", &pidfilename);
if (fd == -1)
- {
- puts ("mkstemp failed");
- exit (1);
- }
+ FAIL_EXIT1 ("create_temp_file failed: %m");
- write (fd, " ", 1);
- close (fd);
+ xwrite (fd, " ", 1);
+ xclose (fd);
}
static int
do_test (void)
{
- pthread_t th;
- if (pthread_create (&th, NULL, tf, NULL) != 0)
- {
- puts ("pthread_create failed");
- return 1;
- }
+ pthread_t th = xpthread_create (NULL, tf, NULL);
do
- sleep (1);
+ nanosleep (&(struct timespec) { .tv_sec = 0, .tv_nsec = 100000000 }, NULL);
while (access (pidfilename, R_OK) != 0);
xpthread_cancel (th);
void *r = xpthread_join (th);
- sleep (1);
+ if (sem_wait (sem) != 0)
+ FAIL_EXIT1 ("sem_wait: %m");
- FILE *f = fopen (pidfilename, "r+");
- if (f == NULL)
- {
- puts ("no pidfile");
- return 1;
- }
+ FILE *f = xfopen (pidfilename, "r+");
long long ll;
if (fscanf (f, "%lld\n", &ll) != 1)
- {
- puts ("could not read pid");
- unlink (pidfilename);
- return 1;
- }
+ FAIL_EXIT1 ("fscanf: %m");
struct flock fl =
{
@@ -136,11 +140,7 @@ do_test (void)
.l_len = 1
};
if (fcntl (fileno (f), F_GETLK, &fl) != 0)
- {
- puts ("F_GETLK failed");
- unlink (pidfilename);
- return 1;
- }
+ FAIL_EXIT1 ("fcntl: %m");
if (fl.l_type != F_UNLCK)
{
@@ -148,13 +148,10 @@ do_test (void)
if (fl.l_pid == ll)
kill (fl.l_pid, SIGKILL);
- unlink (pidfilename);
return 1;
}
- fclose (f);
-
- unlink (pidfilename);
+ xfclose (f);
return r != PTHREAD_CANCELED;
}
@@ -180,15 +177,15 @@ do_cleanup (void)
fclose (f);
}
-
- unlink (pidfilename);
}
#define OPT_COMMAND 10000
#define OPT_PIDFILE 10001
+#define OPT_SEMFILE 10002
#define CMDLINE_OPTIONS \
{ "command", required_argument, NULL, OPT_COMMAND }, \
- { "pidfile", required_argument, NULL, OPT_PIDFILE },
+ { "pidfile", required_argument, NULL, OPT_PIDFILE }, \
+ { "semaphore", required_argument, NULL, OPT_SEMFILE },
static void
cmdline_process (int c)
{
@@ -200,6 +197,9 @@ cmdline_process (int c)
case OPT_PIDFILE:
pidfile = optarg;
break;
+ case OPT_SEMFILE:
+ semfile = optarg;
+ break;
}
}
#define CMDLINE_PROCESS cmdline_process
--
2.30.2
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-06-11 17:24 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-10 19:36 [PATCH 0/6] More NPTL fixes Adhemerval Zanella
2021-06-10 19:36 ` [PATCH 1/6] nptl: Fix tst-cancel7 and tst-cancelx7 race condition (BZ #14232) Adhemerval Zanella
2021-06-11 15:02 ` Florian Weimer
2021-06-11 15:08 ` Adhemerval Zanella
2021-06-11 15:10 ` Florian Weimer
2021-06-11 17:24 ` [PATCH v2 " Adhemerval Zanella
2021-06-10 19:36 ` [PATCH 2/6] nptl: Set cancellation type and state on pthread_exit Adhemerval Zanella
2021-06-10 19:36 ` [PATCH 3/6] nptl: Handle robust PI mutexes for !__ASSUME_SET_ROBUST_LIST Adhemerval Zanella
2021-06-10 19:36 ` [PATCH 4/6] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951) Adhemerval Zanella
2021-06-10 19:36 ` [PATCH 5/6] nptl: Move setxid flag out of cancelhandling Adhemerval Zanella
2021-06-10 19:36 ` [PATCH 6/6] nptl: Replace struct thread cancelhandling field Adhemerval Zanella
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).