[PATCH v5 2/3] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951)
Adhemerval Zanella Netto
adhemerval.zanella@linaro.org
Tue Dec 2 16:54:51 GMT 2025
On 28/11/25 14:17, Florian Weimer wrote:
> * 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?
Ack.
>
>> 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 mean to describe the current situation, not the solution design.
>
> 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)
Ack.
>
>> 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)
Ack.
>
>> 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)
Ack.
>
> Did we base this implementation on the musl approach? If so, maybe
> credit it in the commit message?
Mostly, although I think musl has some structural differences (it does not
have the thread cache, so its synchronization is done differently; it always
calls pthread_exit() on pthread_create(), so most logic in on pthread_exit;
and some other subtle differences). I will add a note about this.
>
>> 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.
Right, I did not use the enum type on joinstate because this version was prior
the atomic cleanup to always use compiler builtins (and I am not sure about
the previous atomic wrapper and enum would work correctly for *all* architectures).
I will remove thread_state_t, I think using an 'unsigned int' clear here.
>
>> /* 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.
Ack.
>
>> 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.
Right, I double-check and it is indeed using tabs here.
>
>> /* 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.
Ack, I will change to 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.
My understanding here in since this code compete with the pthread_detach (issued by
a different thread), the CAS is need to determine which is one responsible to free
the TCB: either the thread itself (below with __nptl_free_tcb), or pthread_detach
with __pthread_join() call.
I am not sure about using an unconditional atomic_exchange, it means will need to
change pthread_detach() to ignore THREAD_STATE_EXITING state and call
__nptl_free_tcb unconditionally. And ignoring THREAD_STATE_EXITINGT makes the
sysdeps/pthread/tst-detach1.c usage tricky to handle.
And I use the acquire because this is the current practice for concurrent
implementations, like __new_sem_wait_fast. But I am not really an concurrency
expert so I am not fully sure if this is the correct MO here.
>
>> @@ -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.
Do mean the '.child_tid = (uintptr_t) &pd->joinstate,' in clone setup? If so, my undertanding
is kernel will only set the value at thread termination.
>
>> @@ -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?
It seems so, I will add an extra patch to fix.
>
> And why acquire MO?
Indeed I think acquire MO is not strickly required here, I will change to relaxed.
>
>> 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.
Ack.
>
> 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?
I am not sure, I tend to avoid adding assert on pthread functions because we
do support some UB (as defined by POSIX) so we can't guarantee that programs
do not rely on such semantics.
>
>> + while (!atomic_compare_exchange_weak_acquire (&pd->joinstate, &prevstate,
>> + THREAD_STATE_DETACHED));
>> + return 0;
>
> Likewise: Why acquire MO?
Right, as pthread_create I am fully sure if relaxed is suffice here. Maybe
change to atomic_compare_exchange_weak_relaxed would be suffice.
>
>> 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?
Ack, I am not really found of this 'may' deadlock check and unfortunately
now that is backed on glibc semantic I did not want to changed it too
much.
>
>> 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)?
I am not sure, the 'joinstate' allows us to determine which is the thread
responsible to free the TCB state; using 'result' will require additional
synchronization and some extra care on its state to certify that we can
assume which thread can actually free the TCB.
>
>> 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.
Before the patch we have the 'block' argument o specify whether to call
__futex_abstimed_wait_cancelable64, now if __pthread_clockjoin_ex is called
'joinstate' is assumed to be THREAD_STATE_EXITED.
The pthread_detach will only change the 'joinstate' if previous state iff
THREAD_STATE_JOINABLE, so even concurrent pthread_detach will not change this
invariant.
>
>> 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.
It makes sense, I will add this.
>
>> 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