diff mbox series

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

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

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Adhemerval Zanella May 26, 2021, 4:57 p.m. UTC 
  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(-)

Comments

Florian Weimer May 26, 2021, 5:38 p.m. UTC | #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
Adhemerval Zanella May 26, 2021, 5:52 p.m. UTC | #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.
diff mbox series

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;