diff mbox series

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

Message ID 20250312191828.173617-1-adhemerval.zanella@linaro.org
State New
Headers show
Series [v2] nptl: Check if thread is already terminated in sigcancel_handler (BZ 32782) | expand

Commit Message

Adhemerval Zanella March 12, 2025, 7:18 p.m. UTC 
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

Comments

Aurelien Jarno March 12, 2025, 9:53 p.m. UTC | #1 
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.
diff mbox series

Patch

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
+     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>