Re: [PATCH v2 09/19] nptl: Fix race between pthread_kill and thread exit (bug 12889)

[Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org>]

On 26/08/2021 11:23, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c
>> index 5d4c86f920..63fe8c44ca 100644
>> --- a/nptl/pthread_kill.c
>> +++ b/nptl/pthread_kill.c
>> @@ -26,15 +26,18 @@ __pthread_kill_internal (pthread_t threadid, int signo)
>>    pid_t tid;
>>    struct pthread *pd = (struct pthread *) threadid;
>>  
>> +  /* Block all signal, since the lock is recursive and used on pthread_cancel
>> +     (which should be async-signal-safe).  */
>> +  sigset_t oldmask;
>> +  __libc_signal_block_all (&oldmask);
>> +  lll_lock (pd->tidlock, LLL_PRIVATE);
>> +
>>    if (pd == THREAD_SELF)
>>      /* It is a special case to handle raise() implementation after a vfork
>>         call (which does not update the PD tid field).  */
>>      tid = INLINE_SYSCALL_CALL (gettid);
>>    else
>> -    /* Force load of pd->tid into local variable or register.  Otherwise
>> -       if a thread exits between ESRCH test and tgkill, we might return
>> -       EINVAL, because pd->tid would be cleared by the kernel.  */
>> -    tid = atomic_forced_read (pd->tid);
>> +    tid = pd->tid;
>>  
>>    int val;
>>    if (__glibc_likely (tid > 0))
>> @@ -53,6 +56,9 @@ __pthread_kill_internal (pthread_t threadid, int signo)
>>         as an error.  */
>>      val = 0;
>>  
>> +  lll_unlock (pd->tidlock, LLL_PRIVATE);
>> +  __libc_signal_restore_set (&oldmask);
>> +
>>    return val;
>>  }
> 
> This needs a comment explaining that *all* pending signals are delivered
> at the point of the __libc_signal_restore_set call.  I hope that this is
> actually what happens in Linux; POSIX only guarantees that for one
> pending signal that is unblocked.

My understanding from kernel/signal.c all pending signals are delivered
with the signal mask restore, but only real-time one are queued (and
subjected to RLIMIT_SIGPENDING, which causes another issue [1]).

> 
> The problem here is that pthread_kill (pthread_self (), …) must generate
> synchronous signal, and due to the signal-blocking, it is not
> immediately obvious if that's the case.
> 
> If we aren't sure that all signals are flushed, we need to check for the
> send-signal-to-self case and avoid blocking the signal there.  We don't
> need the tidlock for that because the running thread won't go away.
> 
> Thanks,
> Florian
> 

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=21108