Re: [PATCH v2 3/9] nptl: Install cancellation handler on pthread_cancel

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 27/05/2021 14:28, Adhemerval Zanella wrote:
> 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.

I also think it fixes BZ#14744, the cancellation handler is now used
solely when pthread_cancel is actually issue.  The sigcancel_handler
already prevents signals from other processes and cancellation from
other processes are an extension that I think it does not really
make sense (as Carlos has pointed ack on the bug report).

> ---
>  nptl/Versions         |  3 +--
>  nptl/pthreadP.h       |  6 ------
>  nptl/pthread_cancel.c | 49 ++++++++++++++++++++++++-------------------
>  nptl/pthread_create.c | 15 -------------
>  4 files changed, 28 insertions(+), 45 deletions(-)
> 
> 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..deb404600c 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 018af30c85..15ce5ad4a1 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -67,21 +67,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.  Use
>       SA_ONSTACK because the signal may be sent to threads that are
>       running with custom stacks.  (This is less likely for
>