[PATCH] nptl: Disable asynchronous cancellation on __do_cancel (BZ 32782)

[Adhemerval Zanella Netto]

On 12/03/25 15:04, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>>>> diff --git a/sysdeps/nptl/pthreadP.h b/sysdeps/nptl/pthreadP.h
>>>> index 2d620ed20d..63de8904f4 100644
>>>> --- a/sysdeps/nptl/pthreadP.h
>>>> +++ b/sysdeps/nptl/pthreadP.h
>>>> @@ -267,8 +267,16 @@ __do_cancel (void *result)
>>>>  
>>>>    self->result = result;
>>>>  
>>>> -  /* Make sure we get no more cancellations.  */
>>>> -  atomic_fetch_or_relaxed (&self->cancelhandling, EXITING_BITMASK);
>>>> +  /* Disable asynchronous cancellation and signal that thread is exiting.  */
>>>> +  int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
>>>> +  int newval;
>>>> +   do
>>>> +     {
>>>> +       newval = (cancelhandling & ~CANCELTYPE_BITMASK) | EXITING_BITMASK;
>>>> +     }
>>>> +   while (!atomic_compare_exchange_weak_acquire (&self->cancelhandling,
>>>> +                                                 &cancelhandling,
>>>> +                                                 newval));
>>>
>>> Unecessary extra braces.
>>
>> Do you mean just:
>>
>>    int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
>>    int newval;
>>    do
>>      newval = (cancelhandling & ~CANCELTYPE_BITMASK) | EXITING_BITMASK;
>>    while (!atomic_compare_exchange_weak_acquire (&self->cancelhandling,
>>                                                  &cancelhandling,
>>                                                  newval));
>>
>> ?
> 
> Exactly.
> 
>>> The cause and nature of the change match my expectations, I merely
>>> wonder how the behavior compares to glibc before your cancellation fix
>>> was applied.
>>
>> Before the bz12683 fix, the SIGCANCEL handler also did not act upon
>> cancellation if the thread was already cancelled:
>>
>>  45   int oldval = atomic_load_relaxed (&self->cancelhandling);
>>  46   while (1)
>>  47     {
>>  48       /* We are canceled now.  When canceled by another thread this flag
>>  49          is already set but if the signal is directly send (internally or
>>  50          from another process) is has to be done here.  */
>>  51       int newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
>>  52
>>  53       if (oldval == newval || (oldval & EXITING_BITMASK) != 0)
>>  54         /* Already canceled or exiting.  */
>>  55         break;
>>  56
>>  57       if (atomic_compare_exchange_weak_acquire (&self->cancelhandling,
>>  58                                                 &oldval, newval))
>>  59         {
>>  60           self->result = PTHREAD_CANCELED;
>>  61
>>  62           /* Make sure asynchronous cancellation is still enabled.  */
>>  63           if ((oldval & CANCELTYPE_BITMASK) != 0)
>>  64             /* Run the registered destructors and terminate the thread.  */
>>  65             __do_cancel ();
>>  66         }
>>  67     }
>>
>> The different was it checked the EXITING_BITMASK.  Another possibility
>> would to add a function like cancel_async_enabled_and_not_cancelled
>> and check it instead of cancel_async_enabled; but I think this is
>> slight more clear (since the same logic is already used on
>> __pthread_unwind).
> 
> There's still an observable difference: We used to disable cancellation
> (state == 0), but we still preserve the async cancel state (state == 1).
> The difference is observable in cancellation handlers (tf_cleanup in the
> test case).
> 
> The difference should not matter in practice, but maybe we should
> reproduce the previous behavior and not clear the async cancel flag?

Indeed I though about that, I will change to check for EXITING_BITMASK
and keep the async state unchanged.