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

[Florian Weimer]

* 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?

Thanks,
Florian