[PATCH v2] nptl: Check if thread is already terminated in sigcancel_handler (BZ 32782)

[Aurelien Jarno]

Hi,

On 2025-03-12 16:18, Adhemerval Zanella wrote:
> The SIGCANCEL signal handler should not issue __syscall_do_cancel,
> which calls __do_cancel and __pthread_unwind, if the cancellation
> is already in proces (and libgcc unwind is not reentrant).  Any
> cancellation signal received after is ignored.
> 
> Checked on x86_64-linux-gnu and aarch64-linux-gnu.
> ---
>  nptl/pthread_cancel.c          | 14 ++++---
>  sysdeps/pthread/Makefile       |  1 +
>  sysdeps/pthread/tst-cancel32.c | 73 ++++++++++++++++++++++++++++++++++
>  3 files changed, 82 insertions(+), 6 deletions(-)
>  create mode 100644 sysdeps/pthread/tst-cancel32.c
> 
> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
> index f7ce3ec51b..79309163eb 100644
> --- a/nptl/pthread_cancel.c
> +++ b/nptl/pthread_cancel.c
> @@ -41,15 +41,17 @@ sigcancel_handler (int sig, siginfo_t *si, void *ctx)
>        || si->si_code != SI_TKILL)
>      return;
>  
> -  /* Check if asynchronous cancellation mode is set or if interrupted
> -     instruction pointer falls within the cancellable syscall bridge.  For
> -     interruptable syscalls with external side-effects (i.e. partial reads),
> -     the kernel  will set the IP to after __syscall_cancel_arch_end, thus
> -     disabling the cancellation and allowing the process to handle such
> +  /* Check if asynchronous cancellation mode is set and cancellation is not
> +     already in progress, or if interrupted instruction pointer falls within
> +     the cancellable syscall bridge.
> +     Forinterruptable syscalls with external side-effects (i.e. partial

Small typo: For interruptable

> +     reads), the kernel will set the IP to after __syscall_cancel_arch_end,
> +     thus disabling the cancellation and allowing the process to handle such
>       conditions.  */
>    struct pthread *self = THREAD_SELF;
>    int oldval = atomic_load_relaxed (&self->cancelhandling);
> -  if (cancel_async_enabled (oldval) || cancellation_pc_check (ctx))
> +  if (cancel_enabled_and_canceled_and_async (oldval)
> +      || cancellation_pc_check (ctx))
>      __syscall_do_cancel ();
>  }
>  
> 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..ab550c16bf
> --- /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 possible failure for pthread_setcanceltype is an
> +	 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>

Thanks a lot of the fix, it was fast. I have tested it, and it fixes
both the original issue in PARI/GP and the reduced testcase, so:

Tested-by: Aurelien Jarno <aurelien@aurel32.net>

I do not feel qualified to review the patch itself, but at least this
matches my observations from debugging, and your explanations make
sense. And the testcase also looks good.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                     http://aurel32.net