Re: [PATCH v2 12/19] nptl: Use tidlock when accessing TID on pthread_getcpuclockid

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

* Adhemerval Zanella:

> On 26/08/2021 11:27, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> Checked on x86_64-linux-gnu.
>>> ---
>>>  nptl/pthread_getcpuclockid.c | 27 +++++++++++++++++----------
>>>  1 file changed, 17 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/nptl/pthread_getcpuclockid.c b/nptl/pthread_getcpuclockid.c
>>> index 0a6656ea4c..8c0db0b9ba 100644
>>> --- a/nptl/pthread_getcpuclockid.c
>>> +++ b/nptl/pthread_getcpuclockid.c
>>> @@ -29,16 +29,23 @@ __pthread_getcpuclockid (pthread_t threadid, clockid_t *clockid)
>>>    struct pthread *pd = (struct pthread *) threadid;
>>>  
>>>    /* Make sure the descriptor is valid.  */
>>> -  if (INVALID_TD_P (pd))
>>> -    /* Not a valid thread handle.  */
>>> -    return ESRCH;
>>> -
>>> -  /* The clockid_t value is a simple computation from the TID.  */
>>> -
>>> -  const clockid_t tidclock = MAKE_THREAD_CPUCLOCK (pd->tid, CPUCLOCK_SCHED);
>>> -
>>> -  *clockid = tidclock;
>>> -  return 0;
>>> +  sigset_t oldmask;
>>> +  __libc_signal_block_all (&oldmask);
>>> +  lll_lock (pd->tidlock, LLL_PRIVATE);
>>> +
>>> +  int res;
>>> +  if (pd->tid != 0)
>>> +    {
>>> +      *clockid = MAKE_THREAD_CPUCLOCK (pd->tid, CPUCLOCK_SCHED);
>>> +      res = 0;
>>> +    }
>>> +  else
>>> +    res = -ESRCH;
>>> +
>>> +  lll_unlock (pd->tidlock, LLL_PRIVATE);
>>> +  __libc_signal_restore_set (&oldmask);
>>> +
>>> +  return res;
>> 
>> This doesn't really solve the race, does it?  The caller cannot use the
>> clock ID safely.
>
> Not really if the callee intention is to use along clock_gettime()
> or any other routine.  I added this patch more for consistency.
>
> In fact, it makes me wonder if the requested improvement such as
> a pthread pid accessor [1] does really make sense, since it is
> inherent racy in a way that there is no guarantee that the tid
> will be valid when it is actually used.
>
> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=27880

I think it still makes sense because the caller can ensure that the
thread does not exit, so the TID remains valid while it is used.

The original TID (after the thread has exited) may also be useful for
debug logs.

Thanks,
Florian