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:

>>>>> 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.
>
> My understanding it we still need to clear the tid member to avoid the
> unintentional usage, since kernel will clear it anymore.  For instance
> on pthread_kill() (or any other usage), there is still an windows
> where the joinstate test and the tid read might result in an invalid
> value (either a tid reuse or an invalid value).

It's more a matter of the final state.  I think we should let the tid
field be after the other changes went in.

Thanks,
Florian