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

  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(-)
  

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
>
  

* Adhemerval Zanella via Libc-alpha:

> 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).

I'm not sure if bug 14744 actually exists, given the checks we have in
the cancellation signal handler.  Particularly the si_pid check should
block acting on signals sent from another process.  And locally, we
filter out the NPTL signals in the system call wrapppers (and we
consider direct system calls undefined in this context).

Thanks,
Florian
  

* Adhemerval Zanella via Libc-alpha:

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

This patch looks okay to me.

Thanks,
Florian
  

On 01/06/2021 05:38, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> 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).
> 
> I'm not sure if bug 14744 actually exists, given the checks we have in
> the cancellation signal handler.  Particularly the si_pid check should
> block acting on signals sent from another process.  And locally, we
> filter out the NPTL signals in the system call wrapppers (and we
> consider direct system calls undefined in this context).

Indeed there is not the issue of external processes cancelling random
threads by throwing SIGCANCEL to random tids, I was referring to the
robustness suggestion to just install the signal handler when
pthread_cancel is actually used as per comment #1 (although it would
only help by invoking the default signal handler).