Re: [PATCH v2 04/19] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951)

[Florian Weimer via Libc-alpha <libc-alpha@sourceware.org>]

* Adhemerval Zanella:

>>> 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()).
>> 
>> See above regarding thread self-deallocation.
>> 
>> The design as described above looks sound to me, those are just nits.
>
> Right, should I change this paragraph as well (it is not clear the
> suggestion here).

Maybe “up to [the] thread itself to [trigger deallocation of] its memory”?

>>> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
>>> index 08e5189ad6..763e32bc3e 100644
>>> --- a/nptl/pthread_create.c
>>> +++ b/nptl/pthread_create.c
>>> @@ -286,7 +286,7 @@ static int create_thread (struct pthread *pd,
>>> const struct
>>> @@ -351,13 +351,16 @@ 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);
>>>  
>>>        if (setup_failed)
>>> -	goto out;
>>> +	{
>>> +	  pd->tid = 0;
>>> +	  goto out;
>>> +	}
>>>      }
>> 
>> What's the advantage of setting pd->tid here and below in start_thread?
>
> We don't really need to clear the tid on setup_failed case in fact, since
> in this case no pthread_t will be returned to caller.  I remove it.

What about the change in start_thread?

The subsequent changes look at the tid member, but they could equally
well look at joinstate, I think.


>> I think you need a strong CAS here.  We don't have, so you'll have to
>> add a loop.
>
> Yeah, it seems right.  I changed to:
>
>   unsigned int prevstate; 
>   while (!atomic_compare_exchange_weak_acquire (&pd->joinstate, &prevstate,
>                                                 THREAD_STATE_EXITING))
>     prevstate = atomic_load_relaxed (&pd->joinstate);

Isn't prevstate uninitialized?  Why no do-while loop?

>> pthread_tryjoin_np on a thread which is THREAD_STATE_DETACHED is
>> invalid, so that case doesn't matter, I think.
>
> I changed the comment to:
>
>   /* The joinable state (THREAD_STATE_JOINABLE) is straigthforward since the
>      thread hasn't finished yet and trying to join might block.
>      The exiting thread (THREAD_STATE_EXITING) also mgith result in ablocking
>      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).  */

Typo: mgith

Rest looks okay to me.

>>> 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
>> 
>>> -  if (thrd_join (id, NULL) == thrd_success)
>>> -    FAIL_EXIT1 ("thrd_join succeed where it should fail");
>>> +  TEST_COMPARE (thrd_join (id, NULL), thrd_error);
>> 
>> This is still a user-after-free bug, right?
>
> Indeed, I think it would be better to just remove this test.

Agreed.

Thanks,
Florian