[PATCH v6 2/4] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951)

[Adhemerval Zanella Netto]

On 08/12/25 19:19, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> @@ -496,6 +498,19 @@ 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
>> +     THREAD_STATE_EXITING state set here ensures that pthread_join() waits
>> +     until all required cleanup steps are complete.
>> +
>> +     The 'prevstate' variable field will be used to determine who is
>> +     responsible for calling __nptl_free_tcb below.  */
>> +
>> +  unsigned int prevstate = atomic_exchange_acquire (&pd->joinstate,
>> +						    THREAD_STATE_EXITING);
> 
> The acquire seems unnecessary because it does not synchronize with
> anything.

Ack, I will change to atomic_exchange_relaxed.

> 
>> @@ -865,10 +883,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_relaxed (&pd->joinstate))
>> +                 != THREAD_STATE_EXITED)
>> +	    __futex_abstimed_wait_cancelable64 (&pd->joinstate, state, 0,
>> +                                                NULL, LLL_SHARED);
>>          }
> 
> Shouldn't use a cancelable wait because pthread_create is declared
> _THROWNL.  Maybe file a separate bug if you don't want to change this
> now.

As you noted I fixed on the last patch.  I will move the fix as the
first patch to avoid further confusion and open a bug report to 
track it.

> 
>> diff --git a/nptl/pthread_detach.c b/nptl/pthread_detach.c
>> index 3bbc037bdc..9b776ba9b0 100644
>> --- a/nptl/pthread_detach.c
>> +++ b/nptl/pthread_detach.c
>> @@ -25,32 +25,32 @@ ___pthread_detach (pthread_t th)
>>  {
>>    struct pthread *pd = (struct pthread *) th;
>>  
>> +  /* CONCURRENCY NOTES:
>>  
>> +     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.
>>  
>> +     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
>>      {
>> +      if (prevstate != THREAD_STATE_JOINABLE)
>> +	{
>> +	  if (prevstate == THREAD_STATE_DETACHED)
>> +	    return EINVAL;
>> +	  /* pthread_detach is declared _THROW so it need to call a
>> +	     pthread_join variant that is not an cancellation entrypoint.  */
> 
> typo: not a[] cancellation entrypoint

Ack.

> 
>> 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;
> 
> Should use relaxed, really.

Ack.

> 
>> diff --git a/nptl/pthread_tryjoin.c b/nptl/pthread_tryjoin.c
>> index 54b528fd19..ff5f1e7faa 100644
>> --- a/nptl/pthread_tryjoin.c
>> +++ b/nptl/pthread_tryjoin.c
>> @@ -21,15 +21,24 @@
>>  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).
>> +
>> +     The ‘joinstate’ does not change during the thread lifetime once the
>> +     kernel sets it to THREAD_STATE_EXITED.  The __pthread_clockjoin_ex will
>> +     only call the cancellable futex if state is not THREAD_STATE_EXITED, so
>> +     calling it should be safe wrt not making pthread_tryjoin_np a
>> +     cancellable entrypoint (since it is marked as __THROW).  */
>> +
>> +  struct pthread *pd = (struct pthread *) threadid;
>> +  return atomic_load_acquire (&pd->joinstate) != THREAD_STATE_EXITED
>> +	 ? EBUSY
>> +	 : __pthread_clockjoin_ex (threadid, thread_return, 0, NULL, false);
>>  }
>>  versioned_symbol (libc, __pthread_tryjoin_np, pthread_tryjoin_np, GLIBC_2_34);
> 
> Huh.  Isn't that a bug on its own because pthread_join is required to be
> a cancellation point, so it needs to act on the cancellation whether or
> not it blocks or not?

I think this is pre-existent issue, on current code pthread_join will *not*
on cancellation if some other thread is already waiting on the 'joinid':

 80   else if (__glibc_unlikely (atomic_compare_exchange_weak_acquire (&pd->joinid,
 81                                                                    &self,
 82                                                                    NULL)))
 83     /* There is already somebody waiting for the thread.  */
 84     return EINVAL;

And if the thread has already exited (pd->tid equal to 0):

 99       pid_t tid;
100       while ((tid = atomic_load_acquire (&pd->tid)) != 0)
101         {

The easiest solution would to add an __pthread_testcancel () on
__pthread_clockjoin_ex (). I will open a bug and add a fix for this.

> 
>> diff --git a/sysdeps/nptl/pthreadP.h b/sysdeps/nptl/pthreadP.h
>> index 881a37cedd..88dea3c1e2 100644
>> --- a/sysdeps/nptl/pthreadP.h
>> +++ b/sysdeps/nptl/pthreadP.h
> 
>> @@ -518,8 +517,10 @@ extern int __pthread_setcanceltype (int type, int *oldtype);
>>  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 *,
>> +				   bool)
>>    attribute_hidden;
> 
> Spurious change?
> 

Indeed.

>>  extern int __pthread_sigmask (int, const sigset_t *, sigset_t *);
>>  libc_hidden_proto (__pthread_sigmask);
>> 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
> 
>> @@ -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");
> 
> typo: succeed[ed]
> 

It is pre-existent type, I will add a fix.

> Thanks,
> Florian
>