Re: [PATCH 3/3] posix: Use posix_spawn on system

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 15/09/2018 12:16, Adhemerval Zanella wrote:
> This patch uses posix_spawn on system implementation.  On Linux this has
> the advantage of much lower memory consumption (usually 32 Kb minimum for
> the mmap stack area).
> 
> Although POSIX does not require, glibc system implementation aims to be
> thread and cancellation safe.  While reentracy handling does not require
> any direct change of current strategy, cancellation requires a posix_spawn
> to be cancellable.  This is done by adding an internal
> __posix_spawn_cancellable which does not disable cancellation neither
> change process signal mask.
> 
> The cancellation code is also moved to generic implementation and enabled
> only if SIGCANCEL is defined (similar on how the cancellation handler is
> enabled on nptl-init.c).
> 
> Checked on x86_64-linux-gnu and i686-linux-gnu.

A small fix below (I can resend the patch if required).

> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
> index 85239cedbf..ed5c613e42 100644
> --- a/sysdeps/unix/sysv/linux/spawni.c
> +++ b/sysdeps/unix/sysv/linux/spawni.c
> @@ -138,11 +138,11 @@ __spawni_child (void *arguments)
>    for (int sig = 1; sig < _NSIG; ++sig)
>      {
>        if ((attr->__flags & POSIX_SPAWN_SETSIGDEF)
> -	  && sigismember (&attr->__sd, sig))
> +	  && __sigismember (&attr->__sd, sig))
>  	{
>  	  sa.sa_handler = SIG_DFL;
>  	}
> -      else if (sigismember (&hset, sig))
> +      else if (__sigismember (&hset, sig))
>  	{
>  	  if (__is_internal_signal (sig))
>  	    sa.sa_handler = SIG_IGN;
> @@ -330,10 +330,14 @@ __spawnix (pid_t * pid, const char *file,
>    if (__glibc_unlikely (stack == MAP_FAILED))
>      return errno;
>  
> -  /* Disable asynchronous cancellation.  */
>    int state;
> -  __libc_ptf_call (__pthread_setcancelstate,
> -                   (PTHREAD_CANCEL_DISABLE, &state), 0);
> +  if ((xflags & SPAWN_XFLAGS_ENABLE_CANCEL) == 0)
> +    {
> +      /* Disable asynchronous cancellation.  */
> +      __libc_ptf_call (__pthread_setcancelstate,
> +		       (PTHREAD_CANCEL_DISABLE, &state), 0);
> +      __libc_signal_block_all (&args.oldmask);
> +    }

In fact I think it would be safer to just enable glibc internal signals
instead of current process mask.  I changed it locally to:

  if (xflags & SPAWN_XFLAGS_ENABLE_CANCEL)
    __libc_signal_block_app (&args.oldmask);
  else
    { 
      /* Disable asynchronous cancellation.  */
      __libc_ptf_call (__pthread_setcancelstate,
                       (PTHREAD_CANCEL_DISABLE, &state), 0);
      __libc_signal_block_all (&args.oldmask);
    }


>  
>    /* Child must set args.err to something non-negative - we rely on
>       the parent and child sharing VM.  */
> @@ -347,8 +351,6 @@ __spawnix (pid_t * pid, const char *file,
>    args.envp = envp;
>    args.xflags = xflags;
>  
> -  __libc_signal_block_all (&args.oldmask);
> -
>    /* The clone flags used will create a new child that will run in the same
>       memory space (CLONE_VM) and the execution of calling thread will be
>       suspend until the child calls execve or _exit.
> @@ -389,9 +391,11 @@ __spawnix (pid_t * pid, const char *file,
>    if ((ec == 0) && (pid != NULL))
>      *pid = new_pid;
>  
> -  __libc_signal_restore_set (&args.oldmask);
> -
> -  __libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0);
> +  if ((xflags & SPAWN_XFLAGS_ENABLE_CANCEL) == 0)
> +    {
> +      __libc_signal_restore_set (&args.oldmask);
> +      __libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0);
> +    }

As as before I changed locally to:

  __libc_signal_restore_set (&args.oldmask);
  if ((xflags & SPAWN_XFLAGS_ENABLE_CANCEL) == 0)
    __libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0);