[06/11] nptl: Install cancellation handler on pthread_cancel

Message ID 20210526165728.1772546-7-adhemerval.zanella@linaro.org
State Superseded
Headers show
Series
  • nptl: pthread cancellation refactor
Related show

Commit Message

Siddhesh Poyarekar via Libc-alpha May 26, 2021, 4:57 p.m.
Now that cancellation is not used anymore to handle thread setup
creation failure, the sighandle can be installed only when
pthread_cancel is actually used.

Checked on x86_64-linux-gnu and aarch64-linux-gnu.
---
 nptl/Versions         |  3 +--
 nptl/pthreadP.h       |  6 ------
 nptl/pthread_cancel.c | 49 ++++++++++++++++++++++++-------------------
 nptl/pthread_create.c | 15 -------------
 4 files changed, 28 insertions(+), 45 deletions(-)

-- 
2.30.2

Comments

Siddhesh Poyarekar via Libc-alpha May 26, 2021, 5:38 p.m. | #1
* Adhemerval Zanella via Libc-alpha:

> @@ -134,7 +135,11 @@ __pthread_cancel (pthread_t th)

>  	       call pthread_cancel (pthread_self ()) without calling

>  	       pthread_create, so the signal handler may not have been

>  	       set up for a self-cancel.  */

> -	    sigcancel_handler ();

> +            {

> +              THREAD_SETMEM (pd, result, PTHREAD_CANCELED);

> +              if ((newval & CANCELTYPE_BITMASK) != 0)

> +	        __do_cancel ();

> +            }


The comment is no longer up to date, I think.  Rest of the change looks
okay to me.

Thanks,
Florian
Siddhesh Poyarekar via Libc-alpha May 26, 2021, 5:52 p.m. | #2
On 26/05/2021 14:38, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:

> 

>> @@ -134,7 +135,11 @@ __pthread_cancel (pthread_t th)

>>  	       call pthread_cancel (pthread_self ()) without calling

>>  	       pthread_create, so the signal handler may not have been

>>  	       set up for a self-cancel.  */

>> -	    sigcancel_handler ();

>> +            {

>> +              THREAD_SETMEM (pd, result, PTHREAD_CANCELED);

>> +              if ((newval & CANCELTYPE_BITMASK) != 0)

>> +	        __do_cancel ();

>> +            }

> 

> The comment is no longer up to date, I think.  Rest of the change looks

> okay to me.


Indeed, I will remove it.

Patch

diff --git a/nptl/Versions b/nptl/Versions
index af62a47cca..590761e730 100644
--- a/nptl/Versions
+++ b/nptl/Versions
@@ -395,7 +395,6 @@  libc {
     __nptl_free_tcb;
     __nptl_nthreads;
     __nptl_setxid_sighandler;
-    __nptl_sigcancel_handler;
     __nptl_stack_list_add;
     __nptl_stack_list_del;
     __pthread_attr_copy;
@@ -514,4 +513,4 @@  ld {
      __nptl_initial_report_events;
      __nptl_set_robust_list_avail;
   }
-}
\ No newline at end of file
+}
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index 05f2bae521..48d48e7008 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -569,12 +569,6 @@  libc_hidden_proto (__pthread_attr_setsigmask_internal)
 extern __typeof (pthread_attr_getsigmask_np) __pthread_attr_getsigmask_np;
 libc_hidden_proto (__pthread_attr_getsigmask_np)
 
-/* The cancellation signal handler defined alongside with
-   pthread_cancel.  This is included in statically linked binaries
-   only if pthread_cancel is linked in.  */
-void __nptl_sigcancel_handler (int sig, siginfo_t *si, void *ctx);
-libc_hidden_proto (__nptl_sigcancel_handler)
-
 /* Special versions which use non-exported functions.  */
 extern void __pthread_cleanup_push (struct _pthread_cleanup_buffer *buffer,
 				    void (*routine) (void *), void *arg);
diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
index 802c691874..f264a4096a 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -28,11 +28,19 @@ 
 #include <gnu/lib-names.h>
 #include <sys/single_threaded.h>
 
-/* For asynchronous cancellation we use a signal.  This is the core
-   logic of the signal handler.  */
+/* For asynchronous cancellation we use a signal.  */
 static void
-sigcancel_handler (void)
+sigcancel_handler (int sig, siginfo_t *si, void *ctx)
 {
+  /* Safety check.  It would be possible to call this function for
+     other signals and send a signal from another process.  This is not
+     correct and might even be a security problem.  Try to catch as
+     many incorrect invocations as possible.  */
+  if (sig != SIGCANCEL
+      || si->si_pid != __getpid()
+      || si->si_code != SI_TKILL)
+    return;
+
   struct pthread *self = THREAD_SELF;
 
   int oldval = THREAD_GETMEM (self, cancelhandling);
@@ -66,24 +74,6 @@  sigcancel_handler (void)
     }
 }
 
-/* This is the actually installed SIGCANCEL handler.  It adds some
-   safety checks before performing the cancellation.  */
-void
-__nptl_sigcancel_handler (int sig, siginfo_t *si, void *ctx)
-{
-  /* Safety check.  It would be possible to call this function for
-     other signals and send a signal from another process.  This is not
-     correct and might even be a security problem.  Try to catch as
-     many incorrect invocations as possible.  */
-  if (sig != SIGCANCEL
-      || si->si_pid != __getpid()
-      || si->si_code != SI_TKILL)
-    return;
-
-  sigcancel_handler ();
-}
-libc_hidden_def (__nptl_sigcancel_handler)
-
 int
 __pthread_cancel (pthread_t th)
 {
@@ -94,6 +84,17 @@  __pthread_cancel (pthread_t th)
     /* Not a valid thread handle.  */
     return ESRCH;
 
+  static int init_sigcancel = 0;
+  if (atomic_load_relaxed (&init_sigcancel) == 0)
+    {
+      struct sigaction sa;
+      sa.sa_sigaction = sigcancel_handler;
+      sa.sa_flags = SA_SIGINFO;
+      __sigemptyset (&sa.sa_mask);
+      __libc_sigaction (SIGCANCEL, &sa, NULL);
+      atomic_store_relaxed (&init_sigcancel, 1);
+    }
+
 #ifdef SHARED
   /* Trigger an error if libgcc_s cannot be loaded.  */
   {
@@ -134,7 +135,11 @@  __pthread_cancel (pthread_t th)
 	       call pthread_cancel (pthread_self ()) without calling
 	       pthread_create, so the signal handler may not have been
 	       set up for a self-cancel.  */
-	    sigcancel_handler ();
+            {
+              THREAD_SETMEM (pd, result, PTHREAD_CANCELED);
+              if ((newval & CANCELTYPE_BITMASK) != 0)
+	        __do_cancel ();
+            }
 	  else
 	    {
 	      /* The cancellation handler will take care of marking the
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index d6b907827a..44d135212d 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -68,21 +68,6 @@  late_init (void)
   struct sigaction sa;
   __sigemptyset (&sa.sa_mask);
 
-  /* Install the cancellation signal handler (in static builds only if
-     pthread_cancel has been linked in).  If for some reason we cannot
-     install the handler we do not abort.  Maybe we should, but it is
-     only asynchronous cancellation which is affected.  */
-#ifndef SHARED
-  extern __typeof (__nptl_sigcancel_handler) __nptl_sigcancel_handler
-    __attribute__ ((weak));
-  if (__nptl_sigcancel_handler != NULL)
-#endif
-    {
-      sa.sa_sigaction = __nptl_sigcancel_handler;
-      sa.sa_flags = SA_SIGINFO;
-      (void) __libc_sigaction (SIGCANCEL, &sa, NULL);
-    }
-
   /* Install the handle to change the threads' uid/gid.  */
   sa.sa_sigaction = __nptl_setxid_sighandler;
   sa.sa_flags = SA_SIGINFO | SA_RESTART;