Re: [PATCH 01/10] nptl: Perform signal initialization upon pthread_create

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

On 18/05/2021 11:24, Florian Weimer via Libc-alpha wrote:
> Install signal handlers and unblock signals before pthread_create
> creates the first thread.
> ---
>  nptl/Versions         |  5 ++-
>  nptl/nptl-init.c      | 75 ------------------------------------
>  nptl/pthreadP.h       |  6 +++
>  nptl/pthread_cancel.c | 88 ++++++++++++++++++++++++++++++++++++++-----
>  nptl/pthread_create.c | 45 +++++++++++++++++++++-
>  5 files changed, 131 insertions(+), 88 deletions(-)
> 
> diff --git a/nptl/Versions b/nptl/Versions
> index e7883cbb49..d96b830d05 100644
> --- a/nptl/Versions
> +++ b/nptl/Versions
> @@ -367,8 +367,6 @@ libc {
>      tss_set;
>    }
>    GLIBC_PRIVATE {
> -     __nptl_create_event;
> -     __nptl_death_event;
>      __default_pthread_attr;
>      __default_pthread_attr_lock;
>      __futex_abstimed_wait64;
> @@ -386,11 +384,14 @@ libc {
>      __lll_trylock_elision;
>      __lll_unlock_elision;
>      __mutex_aconf;
> +    __nptl_create_event;
>      __nptl_deallocate_stack;
>      __nptl_deallocate_tsd;
> +    __nptl_death_event;
>      __nptl_free_tcb;
>      __nptl_nthreads;
>      __nptl_setxid_sighandler;
> +    __nptl_sigcancel_handler;
>      __nptl_stack_list_add;
>      __nptl_stack_list_del;
>      __pthread_attr_copy;
> diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
> index f4b86fbfaf..bc4831ac89 100644
> --- a/nptl/nptl-init.c
> +++ b/nptl/nptl-init.c
> @@ -44,84 +44,9 @@ size_t __static_tls_align_m1;
>  /* Version of the library, used in libthread_db to detect mismatches.  */
>  static const char nptl_version[] __attribute_used__ = VERSION;
>  
> -/* For asynchronous cancellation we use a signal.  This is the handler.  */
> -static 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);
> -  while (1)
> -    {
> -      /* We are canceled now.  When canceled by another thread this flag
> -	 is already set but if the signal is directly send (internally or
> -	 from another process) is has to be done here.  */
> -      int newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
> -
> -      if (oldval == newval || (oldval & EXITING_BITMASK) != 0)
> -	/* Already canceled or exiting.  */
> -	break;
> -
> -      int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
> -					      oldval);
> -      if (curval == oldval)
> -	{
> -	  /* Set the return value.  */
> -	  THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> -
> -	  /* Make sure asynchronous cancellation is still enabled.  */
> -	  if ((newval & CANCELTYPE_BITMASK) != 0)
> -	    /* Run the registered destructors and terminate the thread.  */
> -	    __do_cancel ();
> -
> -	  break;
> -	}
> -
> -      oldval = curval;
> -    }
> -}
> -
> -
> -/* When using __thread for this, we do it in libc so as not
> -   to give libpthread its own TLS segment just for this.  */
> -extern void **__libc_dl_error_tsd (void) __attribute__ ((const));
> -
> -
>  void
>  __pthread_initialize_minimal_internal (void)
>  {
> -  struct sigaction sa;
> -  __sigemptyset (&sa.sa_mask);
> -
> -  /* Install the cancellation signal handler.  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.  */
> -  sa.sa_sigaction = 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;
> -  (void) __libc_sigaction (SIGSETXID, &sa, NULL);
> -
> -  /* The parent process might have left the signals blocked.  Just in
> -     case, unblock it.  We reuse the signal mask in the sigaction
> -     structure.  It is already cleared.  */
> -  __sigaddset (&sa.sa_mask, SIGCANCEL);
> -  __sigaddset (&sa.sa_mask, SIGSETXID);
> -  INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_UNBLOCK, &sa.sa_mask,
> -			 NULL, __NSIG_BYTES);
> -
>    /* Get the size of the static and alignment requirements for the TLS
>       block.  */
>    size_t static_tls_align;
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index f93806e540..497c2ad3d9 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -571,6 +571,12 @@ 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 e4ad602900..802c691874 100644
> --- a/nptl/pthread_cancel.c
> +++ b/nptl/pthread_cancel.c
> @@ -26,6 +26,63 @@
>  #include <unwind-link.h>
>  #include <stdio.h>
>  #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.  */
> +static void
> +sigcancel_handler (void)
> +{
> +  struct pthread *self = THREAD_SELF;
> +
> +  int oldval = THREAD_GETMEM (self, cancelhandling);
> +  while (1)
> +    {
> +      /* We are canceled now.  When canceled by another thread this flag
> +	 is already set but if the signal is directly send (internally or
> +	 from another process) is has to be done here.  */
> +      int newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
> +
> +      if (oldval == newval || (oldval & EXITING_BITMASK) != 0)
> +	/* Already canceled or exiting.  */
> +	break;
> +
> +      int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
> +					      oldval);
> +      if (curval == oldval)
> +	{
> +	  /* Set the return value.  */
> +	  THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> +
> +	  /* Make sure asynchronous cancellation is still enabled.  */
> +	  if ((newval & CANCELTYPE_BITMASK) != 0)
> +	    /* Run the registered destructors and terminate the thread.  */
> +	    __do_cancel ();
> +
> +	  break;
> +	}
> +
> +      oldval = curval;
> +    }
> +}
> +
> +/* 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)
> @@ -72,14 +129,23 @@ __pthread_cancel (pthread_t th)
>  						    oldval))
>  	    goto again;
>  
> -	  /* The cancellation handler will take care of marking the
> -	     thread as canceled.  */
> -	  pid_t pid = __getpid ();
> -
> -	  int val = INTERNAL_SYSCALL_CALL (tgkill, pid, pd->tid,
> -					   SIGCANCEL);
> -	  if (INTERNAL_SYSCALL_ERROR_P (val))
> -	    result = INTERNAL_SYSCALL_ERRNO (val);
> +	  if (pd == THREAD_SELF)
> +	    /* This is not merely an optimization: An application may
> +	       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 ();

I think it would be simple to just call __pthread_exit (PTHREAD_CANCELED)
here, it won't require to split the cancellation handler, it already
unwind if cancel state is enabled and asynchronous, and it does not
require add another PTHREAD_STATIC_FN_REQUIRE hack. 

It would require an extra __libc_unwind_link_get call, but I think we
can optimize it later (I am working on a patch to simplify it).

> +	  else
> +	    {
> +	      /* The cancellation handler will take care of marking the
> +		 thread as canceled.  */
> +	      pid_t pid = __getpid ();
> +
> +	      int val = INTERNAL_SYSCALL_CALL (tgkill, pid, pd->tid,
> +					       SIGCANCEL);
> +	      if (INTERNAL_SYSCALL_ERROR_P (val))
> +		result = INTERNAL_SYSCALL_ERRNO (val);
> +	    }
>  
>  	  break;
>  	}
> @@ -106,4 +172,8 @@ versioned_symbol (libc, __pthread_cancel, pthread_cancel, GLIBC_2_34);
>  compat_symbol (libpthread, __pthread_cancel, pthread_cancel, GLIBC_2_0);
>  #endif
>  
> -PTHREAD_STATIC_FN_REQUIRE (__pthread_create)
> +/* Ensure that the unwinder is always linked in (the __pthread_unwind
> +   reference from __do_cancel is weak).  Use ___pthread_unwind_next
> +   (three underscores) to produce a strong reference to the same
> +   file.  */
> +PTHREAD_STATIC_FN_REQUIRE (___pthread_unwind_next)
> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index 770656453d..772b5efcc6 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -56,6 +56,43 @@ static struct rtld_global *__nptl_rtld_global __attribute_used__
>    = &_rtld_global;
>  #endif
>  
> +/* This performs the initialization necessary when going from
> +   single-threaded to multi-threaded mode for the first time.  */
> +static void
> +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

This weak symbol can be avoided if we move the cancellation setup
on pthread_cancel instead.  I still think this is best approach,
it disentangle the cancellation handling.

> +    {
> +      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;
> +  (void) __libc_sigaction (SIGSETXID, &sa, NULL);
> +
> +  /* The parent process might have left the signals blocked.  Just in
> +     case, unblock it.  We reuse the signal mask in the sigaction
> +     structure.  It is already cleared.  */
> +  __sigaddset (&sa.sa_mask, SIGCANCEL);
> +  __sigaddset (&sa.sa_mask, SIGSETXID);
> +  INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_UNBLOCK, &sa.sa_mask,
> +			 NULL, __NSIG_BYTES);
> +}
> +
>  /* Code to allocate and deallocate a stack.  */
>  #include "allocatestack.c"
>  
> @@ -459,9 +496,13 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>  {
>    STACK_VARIABLES;
>  
> -  /* Avoid a data race in the multi-threaded case.  */
> +  /* Avoid a data race in the multi-threaded case, and call the
> +     deferred initialization only once.  */
>    if (__libc_single_threaded)
> -    __libc_single_threaded = 0;
> +    {
> +      late_init ();
> +      __libc_single_threaded = 0;
> +    }
>  
>    const struct pthread_attr *iattr = (struct pthread_attr *) attr;
>    union pthread_attr_transparent default_attr;
>