| Message ID | 20250312140544.3569550-1-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | nptl: Disable asynchronous cancellation on __do_cancel (BZ 32782) | expand |
* 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. 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. > 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[] Thanks, Florian
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 >
* 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
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.
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)); __pthread_unwind ((__pthread_unwind_buf_t *) THREAD_GETMEM (self, cleanup_jmp_buf)); diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile index 70e62b2e1b..d4869c624b 100644 --- a/sysdeps/pthread/Makefile +++ b/sysdeps/pthread/Makefile @@ -106,6 +106,7 @@ tests += \ tst-cancel28 \ tst-cancel29 \ tst-cancel30 \ + tst-cancel32 \ tst-cleanup0 \ tst-cleanup1 \ tst-cleanup2 \ 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 @@ -0,0 +1,73 @@ +/* Check if pthread_setcanceltype disables asynchronous cancellation + once cancellation happens (BZ 32782) + + Copyright (C) 2025 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +/* The pthread_setcanceltype is a cancellation entrypoint, and if + asynchronous is enabled and the cancellation starts (on the second + pthread_setcanceltype call), the asynchronous should not restart + the process. */ + +#include <support/xthread.h> + +#define NITER 1000 +#define NTHREADS 8 + +static void +tf_cleanup (void *arg) +{ +} + +static void * +tf (void *closure) +{ + pthread_cleanup_push (tf_cleanup, NULL); + for (;;) + { + /* The only failure possible for pthread_setcanceltype is and + invalid state type. */ + pthread_setcanceltype (PTHREAD_CANCEL_ASYNCHRONOUS, NULL); + pthread_setcanceltype (PTHREAD_CANCEL_DEFERRED, NULL); + } + pthread_cleanup_pop (1); + + return NULL; +} + +static void +poll_threads (int nthreads) +{ + pthread_t thr[nthreads]; + for (int i = 0; i < nthreads; i++) + thr[i] = xpthread_create (NULL, tf, NULL); + for (int i = 0; i < nthreads; i++) + xpthread_cancel (thr[i]); + for (int i = 0; i < nthreads; i++) + xpthread_join (thr[i]); +} + +static int +do_test (void) +{ + for (int k = 0; k < NITER; k++) + poll_threads (NTHREADS); + + return 0; +} + +#include <support/test-driver.c>