Re: [PATCH v2] nptl: Handle spurious EINTR when thread cancellation is disabled (BZ#29029)

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 12/07/22 18:27, Noah Goldstein wrote:
>> diff --git a/nptl/cleanup_defer.c b/nptl/cleanup_defer.c
>> index f8181a40e8..eb0bc77740 100644
>> --- a/nptl/cleanup_defer.c
>> +++ b/nptl/cleanup_defer.c
>> @@ -30,9 +30,22 @@ ___pthread_register_cancel_defer (__pthread_unwind_buf_t *buf)
>>     ibuf->priv.data.prev = THREAD_GETMEM (self, cleanup_jmp_buf);
>>     ibuf->priv.data.cleanup = THREAD_GETMEM (self, cleanup);
>>
>> -  /* Disable asynchronous cancellation for now.  */
>> -  ibuf->priv.data.canceltype = THREAD_GETMEM (self, canceltype);
>> -  THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_DEFERRED);
>> +  int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
>> +  if (__glibc_unlikely (cancelhandling & CANCELTYPE_BITMASK))
>> +    {
>> +      int newval;
>> +      do
>> +       {
>> +         newval = cancelhandling & ~CANCELTYPE_BITMASK;
>> +       }
>> +      while (!atomic_compare_exchange_weak_acquire (&self->cancelhandling,
>> +                                                   &cancelhandling,
>> +                                                   newval));
>> +    }
>> +
>> +  ibuf->priv.data.canceltype = (cancelhandling & CANCELTYPE_BITMASK
>> +                               ? PTHREAD_CANCEL_ASYNCHRONOUS
>> +                               : PTHREAD_CANCEL_DEFERRED);
>>
>>     /* Store the new cleanup handler info.  */
>>     THREAD_SETMEM (self, cleanup_jmp_buf, (struct pthread_unwind_buf *) buf);
>> @@ -54,9 +67,26 @@ ___pthread_unregister_cancel_restore (__pthread_unwind_buf_t *buf)
>>
>>     THREAD_SETMEM (self, cleanup_jmp_buf, ibuf->priv.data.prev);
>>
>> -  THREAD_SETMEM (self, canceltype, ibuf->priv.data.canceltype);
>> -  if (ibuf->priv.data.canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
>> -    __pthread_testcancel ();
>> +  if (ibuf->priv.data.canceltype == PTHREAD_CANCEL_DEFERRED)
>> +    return;
>> +
>> +  int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
>> +  if (cancelhandling & CANCELTYPE_BITMASK)
> 
> should this be:
> if((cancelhandling & CANCELTYPE_BITMASK) == 0)
> ?

Yes, I will fix it.