[PATCH v5 2/3] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951)
Florian Weimer
fweimer@redhat.com
Fri Nov 28 17:17:03 GMT 2025
* Adhemerval Zanella:
> The use-after-free described in BZ#19951 is due to the use of two
> different PD fields, 'joinid' and 'cancelhandling', to describe the
Maybe: different struct pthread fields?
> thread state and to synchronise the calls of pthread_join,
> pthread_detach, pthread_exit, and normal thread exit.
>
> Any state change may require checking both fields atomically to handle
> partial state (e.g., pthread_join() with a cancellation handler to
> issue a 'joinstate' field rollback).
I think this needs some revision because I don't think it's required to
check both fields atomically at the same time.
> This patch uses a different PD member with 4 possible states (JOINABLE,
(again PD)
> DETACHED, EXITING, and EXITED) instead of the pthread 'tid' field, with
> the following logic:
>
> 1. On pthread_create, the initial state is set either to JOINABLE or
> DETACHED depending on the pthread attribute used.
>
> 2. On pthread_detach, a CAS is issued on the state. If the CAS fails,
> the thread is already detached (DETACHED) or being terminated (EXITING).
> For the former, an EINVAL is returned; for the latter, pthread_detach
> should be responsible for joining the thread (and for deallocating any
> internal resources).
>
> 3. In the exit phase of the wrapper function for the thread start routine
> (reached either if the thread function has returned, pthread_exit has
> been called, or cancellation handled has been acted upon), we issue a
> CAS on state to set it to the EXITING mode.
>
> If the thread is previously in DETACHED mode, the thread is responsible
> for deallocating any resources; otherwise, the thread must be joined
> (detached threads cannot deallocate themselves immediately).
>
> 4. The clear_tid_field on 'clone' call is changed to set the new 'state'
> field on thread exit (EXITED). This state is only reached at thread
> termination.
>
> 5. 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 a single atomic
> operation on the PD state: once the mode is set to THREAD_STATE_DETACHED, it
(PD)
> is up to the thread itself to deallocate its memory (done during the exit
> phase at pthread_create()).
>
> Also, the INVALID_NOT_TERMINATED_TD_P is removed since a negative yid is
> not possible, and the macro is not used anywhere.
typo: negative [t]id (or rather: TID)
Did we base this implementation on the musl approach? If so, maybe
credit it in the commit message?
> diff --git a/nptl/descr.h b/nptl/descr.h
> index ada6867a19..396b2aa008 100644
> --- a/nptl/descr.h
> +++ b/nptl/descr.h
> @@ -132,6 +132,18 @@ enum allocate_stack_mode_t
> ALLOCATE_GUARD_USER = 2,
> };
>
> +/* Possible values for the 'joinstate' field. The field will be cleared
> + (set to THREAD_STATE_EXITED) atomically by the kernel when thread
> + terminated. */
> +enum thread_state_t
> +{
> + THREAD_STATE_EXITED = 0,
> + THREAD_STATE_EXITING,
> + THREAD_STATE_JOINABLE,
> + THREAD_STATE_DETACHED,
> +};
> +
> +
> /* Thread descriptor data structure. */
> struct pthread
> {
> @@ -174,8 +186,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;
>
> /* List of robust mutexes the thread is holding. */
> @@ -345,15 +356,8 @@ struct pthread
> /* Lock for synchronizing setxid calls. */
> unsigned int setxid_futex;
>
> - /* 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;
Either use enum thread_state_t here (maybe problematic because of atomic
access), or remove the thread_state_t tag from the enum definition.
> /* Remove the stack ELEM from its list. */
> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
> index b838273881..575f29f068 100644
> --- a/nptl/pthread_cancel.c
> +++ b/nptl/pthread_cancel.c
> @@ -60,7 +60,8 @@ __pthread_cancel (pthread_t th)
> {
> volatile struct pthread *pd = (volatile struct pthread *) th;
>
> - if (pd->tid == 0)
> + int state = atomic_load_relaxed (&pd->joinstate);
The joinstate field has type unsigned int.
> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index 19e4ec8064..ab210285cd 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -290,7 +290,7 @@ static int create_thread (struct pthread *pd, const struct pthread_attr *attr,
> .flags = clone_flags,
> .pidfd = (uintptr_t) &pd->tid,
> .parent_tid = (uintptr_t) &pd->tid,
> - .child_tid = (uintptr_t) &pd->tid,
> + .child_tid = (uintptr_t) &pd->joinstate,
> .stack = (uintptr_t) stackaddr,
> .stack_size = stacksize,
> .tls = (uintptr_t) tp,
> @@ -355,12 +355,14 @@ 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;
Ah, confusing indentation (in the diff only). It's for the failed case.
> /* And give it up right away. */
> lll_unlock (pd->lock, LLL_PRIVATE);
>
> if (setup_failed)
> + /* No need to clear the tid here, pthread_create() will join the
> + thread prior returning to caller. */
> goto out;
> }
>
> @@ -496,6 +498,22 @@ start_thread (void *arg)
> the breakpoint reports TD_THR_RUN state rather than TD_THR_ZOMBIE. */
> atomic_fetch_or_relaxed (&pd->cancelhandling, EXITING_BITMASK);
>
> + /* CONCURRENCY NOTES:
> +
> + Concurrent pthread_detach() either sets the state to
> + THREAD_STATE_DETACHED or waits for the thread to terminate. The existing
> + state set here ensures that pthread_join() waits until all required
> + cleanup steps are complete.
Existing or exiting? Maybe write THREAD_STATE_EXITING.
> + The 'prevstate' field will be used to determine who is responsible for
> + calling __nptl_free_tcb below. */
typo: The 'prevstate' [variable] …
> + unsigned int prevstate;
> + do
> + prevstate = atomic_load_relaxed (&pd->joinstate);
> + while (!atomic_compare_exchange_weak_acquire (&pd->joinstate, &prevstate,
> + THREAD_STATE_EXITING));
> +
Why acquire MO? I think even if the kernel uses a release store, it's
not exactly clear what we are synchronizing with.
And this looks like an unconditional atomic_exchange.
> @@ -706,7 +725,9 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
> /* 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;
There's also an assignment in create_thread above.
> @@ -865,10 +886,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);
> }
Unrelated: Isn't it a bug that this is cancellation point? Because
pthread_create is declared _THROWNL?
And why acquire MO?
> diff --git a/nptl/pthread_detach.c b/nptl/pthread_detach.c
> index 3bbc037bdc..4c17d51bdd 100644
> --- a/nptl/pthread_detach.c
> +++ b/nptl/pthread_detach.c
> @@ -25,32 +25,28 @@ ___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 where 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))
> + In the case the thread is being terminated (THREAD_STATE_EXITING),
> + pthread_detach will be responsible for cleaning up the stack. */
> +
> + unsigned int prevstate = atomic_load_relaxed (&pd->joinstate);
> + do
> {
> - /* 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 (prevstate != THREAD_STATE_JOINABLE)
> + {
> + if (prevstate == THREAD_STATE_DETACHED)
> + return EINVAL;
> + return __pthread_join (th, 0);
> + }
Our implementation of pthread_detach is declared _THROW, so this needs
to use a pthread_join variant that is not a cancellation point.
Can we put in an assert that checks for prevstate ==
PTHREAD_STATE_EXITING || prevstate == PTHREAD_STATE_EXITED? Or would
that reveal to many bugs in existing applications?
> + while (!atomic_compare_exchange_weak_acquire (&pd->joinstate, &prevstate,
> + THREAD_STATE_DETACHED));
> + return 0;
Likewise: Why acquire MO?
> diff --git a/nptl/pthread_getattr_np.c b/nptl/pthread_getattr_np.c
> index 43dd16d59c..f6d7526041 100644
> --- a/nptl/pthread_getattr_np.c
> +++ b/nptl/pthread_getattr_np.c
> @@ -52,7 +52,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;
Why acquire MO?
> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
> index 9109b62276..f0ae5edbcb 100644
> --- a/nptl/pthread_join_common.c
> +++ b/nptl/pthread_join_common.c
> @@ -22,116 +22,78 @@
> #include <time.h>
> #include <futex-internal.h>
>
> +/* 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 (struct pthread *pd)
> {
> + struct pthread *self = THREAD_SELF;
> + return ((pd == self
> + || (atomic_load_acquire (&self->joinstate) == THREAD_STATE_DETACHED
> + && (pd->cancelhandling
> + & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK
> + | TERMINATED_BITMASK)) == 0))
> + && !cancel_enabled_and_canceled (self->cancelhandling));
> }
Why acquire MO?
Is the condition really correct (beyond pd ==self)? Why is
self->joinstate == THREAD_STATE_DETACHED treated differently here?
I think with pd->joinid gone, there really isn't much we can check here
anymore. Maybe just stick to pd == self and do away with this separate
function altogether?
> int
> __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
> clockid_t clockid,
> + const struct __timespec64 *abstime)
> + int result = 0;
> + unsigned int state;
> + while ((state = atomic_load_acquire (&pd->joinstate))
> + != THREAD_STATE_EXITED)
> {
> + if (check_for_deadlock (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 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 zeroing 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;
> }
> }
>
> void *pd_result = pd->result;
> if (__glibc_likely (result == 0))
> {
> if (thread_return != NULL)
> *thread_return = pd_result;
>
> /* Free the TCB. */
> __nptl_free_tcb (pd);
> }
I'm trying to understand if this leaks the TCB on cancellation. As far
as I can tell, phtread_join needs to be called again if it gets
canceled, so this should be okay.
For synchronization purposes, rather than relying on acquire loads on
joinstate (where the kernel may or may not perform a release store),
maybe we should use an acquire load on pd->result (and a corresponding
release store)?
> diff --git a/nptl/pthread_tryjoin.c b/nptl/pthread_tryjoin.c
> index 54b528fd19..e9a10aee53 100644
> --- a/nptl/pthread_tryjoin.c
> +++ b/nptl/pthread_tryjoin.c
> @@ -21,15 +21,18 @@
> int
> __pthread_tryjoin_np (pthread_t threadid, void **thread_return)
> {
> + /* The joinable state (THREAD_STATE_JOINABLE) is straightforward: the thread
> + hasn't finished yet, so trying to join might block.
> +
> + The exiting thread (THREAD_STATE_EXITING) also might result in a blocking
> + call: a detached thread might change its state to exiting, and an exiting
> + thread might take some time to exit (and thus let the kernel set the
> + state to THREAD_STATE_EXITED). */
>
> + 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);
Pre-existing issue: pthread_tryjoin_np cannot be a cancellation point
because it is declared _THROW.
> diff --git a/sysdeps/nptl/libc_start_call_main.h b/sysdeps/nptl/libc_start_call_main.h
> index ca0436d27a..c1ce90e2f1 100644
> --- a/sysdeps/nptl/libc_start_call_main.h
> +++ b/sysdeps/nptl/libc_start_call_main.h
> @@ -18,6 +18,7 @@
>
> #include <atomic.h>
> #include <pthreadP.h>
> +#include <futex-internal.h>
>
> _Noreturn static void
> __libc_start_call_main (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
> @@ -65,6 +66,11 @@ __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_fetch_add_relaxed (&__nptl_nthreads, -1) != 1)
> /* Not much left to do but to exit the thread, not the process. */
> while (1)
I think we should block signals before we can set THREAD_STATE_EXITED,
otherwise user code can run in that state which should never be
observable as the state of the current thread.
> diff --git a/sysdeps/pthread/tst-thrd-detach.c b/sysdeps/pthread/tst-thrd-detach.c
> index 966e7c1289..fa8d4181f3 100644
> --- a/sysdeps/pthread/tst-thrd-detach.c
> +++ b/sysdeps/pthread/tst-thrd-detach.c
> @@ -28,7 +28,10 @@ detach_thrd (void *arg)
> {
> if (thrd_detach (thrd_current ()) != thrd_success)
> FAIL_EXIT1 ("thrd_detach failed");
> - thrd_exit (thrd_success);
> +
> + pause ();
> +
> + return 0;
> }
>
> static int
> @@ -43,6 +46,7 @@ do_test (void)
> /* Give some time so the thread can finish. */
> thrd_sleep (&(struct timespec) {.tv_sec = 2}, NULL);
>
> + /* Calling thrd_join on a detached thread is UB... */
> if (thrd_join (id, NULL) == thrd_success)
> FAIL_EXIT1 ("thrd_join succeed where it should fail");
Not great, but okay.
Thanks,
Florian
More information about the Libc-alpha
mailing list