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

[Adhemerval Zanella Netto]

On 12/03/25 13:51, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> Similar to __pthread_unwind, called from pthread_exit, once cancellation
>> starts the cancellation signal handler (sigcancel_handler) should not
>> restart the cancellation process (and libgcc unwind is not reentrant).
>> So also disables asynchronous cancellation on __do_cancel, any
>> cancellation signal received after it is ignored (cancel_async_enabled
>> will return false).
> 
> Does this change bring back the previous behavior (in that further
> attempts to cancel are ignored)?
> 
>> 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));

?

> 
> 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).

> 
>> diff --git a/sysdeps/pthread/tst-cancel32.c b/sysdeps/pthread/tst-cancel32.c
>> new file mode 100644
>> index 0000000000..1c6a4d8f47
>> --- /dev/null
>> +++ b/sysdeps/pthread/tst-cancel32.c
> 
>> +static void *
>> +tf (void *closure)
>> +{
>> +  pthread_cleanup_push (tf_cleanup, NULL);
>> +  for (;;)
>> +    {
>> +      /* The only failure possible for pthread_setcanceltype is and
> 
> Typo: is an[]

Ack.


> 
> Thanks,
> Florian
>