Re: [PATCH] linux: spawni.c: simplify error reporting to parent

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

LGTM, I think it addressed all the comments.

On 20/09/2016 18:01, Rasmus Villemoes wrote:
> Using VFORK already ensures that the parent does not run until the
> child has either exec'ed succesfully or called _exit. Hence we don't
> need to read from a CLOEXEC pipe to ensure proper synchronization - we
> just make explicit use of the fact the the child and parent run in the
> same VM, so the child can write an error code to a field of the
> posix_spawn_args struct instead of sending it through a pipe.
> 
> To ensure that this mechanism really works, the parent initializes the
> field to -1 and the child writes 0 before execing.
> 
> This eliminates some annoying bookkeeping that is necessary to avoid
> the file actions from clobbering the write end of the pipe, and
> getting rid of the pipe creation in the first place means fewer system
> calls (four in the parent, usually one in the child) and fewer
> chanches for the spawn to fail (e.g. if we're close to EMFILE).
> 
> Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
> ---
>  sysdeps/unix/sysv/linux/spawni.c | 71 ++++++++++++++--------------------------
>  1 file changed, 25 insertions(+), 46 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
> index bb3eecf..c0e15ac 100644
> --- a/sysdeps/unix/sysv/linux/spawni.c
> +++ b/sysdeps/unix/sysv/linux/spawni.c
> @@ -17,6 +17,7 @@
>     <http://www.gnu.org/licenses/>.  */
>  
>  #include <spawn.h>
> +#include <assert.h>
>  #include <fcntl.h>
>  #include <paths.h>
>  #include <string.h>
> @@ -44,11 +45,12 @@
>     3. Child must synchronize with parent to enforce 2. and to possible
>        return execv issues.
>  
> -   The first issue is solved by blocking all signals in child, even the
> -   NPTL-internal ones (SIGCANCEL and SIGSETXID).  The second and third issue
> -   is done by a stack allocation in parent and a synchronization with using
> -   a pipe or waitpid (in case or error).  The pipe has the advantage of
> -   allowing the child the communicate an exec error.  */
> +   The first issue is solved by blocking all signals in child, even
> +   the NPTL-internal ones (SIGCANCEL and SIGSETXID).  The second and
> +   third issue is done by a stack allocation in parent, and by using a
> +   field in struct spawn_args where the child can write an error
> +   code. CLONE_VFORK ensures that the parent does not run until the
> +   child has either exec'ed successfully or exited.  */
>  
>  
>  /* The Unix standard contains a long explanation of the way to signal
> @@ -79,7 +81,6 @@
>  
>  struct posix_spawn_args
>  {
> -  int pipe[2];
>    sigset_t oldmask;
>    const char *file;
>    int (*exec) (const char *, char *const *, char *const *);
> @@ -89,6 +90,7 @@ struct posix_spawn_args
>    ptrdiff_t argc;
>    char *const *envp;
>    int xflags;
> +  int err;
>  };
>  
>  /* Older version requires that shell script without shebang definition
> @@ -125,11 +127,8 @@ __spawni_child (void *arguments)
>    struct posix_spawn_args *args = arguments;
>    const posix_spawnattr_t *restrict attr = args->attr;
>    const posix_spawn_file_actions_t *file_actions = args->fa;
> -  int p = args->pipe[1];
>    int ret;
>  
> -  close_not_cancel (args->pipe[0]);
> -
>    /* The child must ensure that no signal handler are enabled because it shared
>       memory with parent, so the signal disposition must be either SIG_DFL or
>       SIG_IGN.  It does by iterating over all signals and although it could
> @@ -203,17 +202,6 @@ __spawni_child (void *arguments)
>  	{
>  	  struct __spawn_action *action = &file_actions->__actions[cnt];
>  
> -	  /* Dup the pipe fd onto an unoccupied one to avoid any file
> -	     operation to clobber it.  */
> -	  if ((action->action.close_action.fd == p)
> -	      || (action->action.open_action.fd == p)
> -	      || (action->action.dup2_action.fd == p))
> -	    {
> -	      if ((ret = __dup (p)) < 0)
> -		goto fail;
> -	      p = ret;
> -	    }
> -
>  	  switch (action->tag)
>  	    {
>  	    case spawn_do_close:
> @@ -273,6 +261,7 @@ __spawni_child (void *arguments)
>    __sigprocmask (SIG_SETMASK, (attr->__flags & POSIX_SPAWN_SETSIGMASK)
>  		 ? &attr->__ss : &args->oldmask, 0);
>  
> +  args->err = 0;
>    args->exec (args->file, args->argv, args->envp);
>  
>    /* This is compatibility function required to enable posix_spawn run
> @@ -280,14 +269,13 @@ __spawni_child (void *arguments)
>       (2.15).  */
>    maybe_script_execute (args);
>  
> -  ret = -errno;
> -
>  fail:
> -  /* Since sizeof errno < PIPE_BUF, the write is atomic. */
> -  ret = -ret;
> -  if (ret)
> -    while (write_not_cancel (p, &ret, sizeof ret) < 0)
> -      continue;
> +  /* errno should have an appropriate non-zero value; otherwise,
> +     there's a bug in glibc or the kernel.  For lack of an error code
> +     (EINTERNALBUG) describing that, use ECHILD.  Another option would
> +     be to set args->err to some negative sentinel and have the parent
> +     abort(), but that seems needlessly harsh.  */
> +  args->err = errno ? : ECHILD;
>    _exit (SPAWN_ERROR);
>  }
>  
> @@ -304,9 +292,6 @@ __spawnix (pid_t * pid, const char *file,
>    struct posix_spawn_args args;
>    int ec;
>  
> -  if (__pipe2 (args.pipe, O_CLOEXEC))
> -    return errno;
> -
>    /* To avoid imposing hard limits on posix_spawn{p} the total number of
>       arguments is first calculated to allocate a mmap to hold all possible
>       values.  */
> @@ -333,15 +318,14 @@ __spawnix (pid_t * pid, const char *file,
>    void *stack = __mmap (NULL, stack_size, prot,
>  			MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
>    if (__glibc_unlikely (stack == MAP_FAILED))
> -    {
> -      close_not_cancel (args.pipe[0]);
> -      close_not_cancel (args.pipe[1]);
> -      return errno;
> -    }
> +    return errno;
>  
>    /* Disable asynchronous cancellation.  */
>    int cs = LIBC_CANCEL_ASYNC ();
>  
> +  /* Child must set args.err to something non-negative - we rely on
> +     the parent and child sharing VM.  */
> +  args.err = -1;
>    args.file = file;
>    args.exec = exec;
>    args.fa = file_actions;
> @@ -355,9 +339,8 @@ __spawnix (pid_t * pid, const char *file,
>  
>    /* 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.  These condition as
> -     signal below either by pipe write (_exit with SPAWN_ERROR) or
> -     a successful execve.
> +     suspend until the child calls execve or _exit.
> +
>       Also since the calling thread execution will be suspend, there is not
>       need for CLONE_SETTLS.  Although parent and child share the same TLS
>       namespace, there will be no concurrent access for TLS variables (errno
> @@ -365,22 +348,18 @@ __spawnix (pid_t * pid, const char *file,
>    new_pid = CLONE (__spawni_child, STACK (stack, stack_size), stack_size,
>  		   CLONE_VM | CLONE_VFORK | SIGCHLD, &args);
>  
> -  close_not_cancel (args.pipe[1]);
> -
>    if (new_pid > 0)
>      {
> -      if (__read (args.pipe[0], &ec, sizeof ec) != sizeof ec)
> -	ec = 0;
> -      else
> -	__waitpid (new_pid, NULL, 0);
> +      ec = args.err;
> +      assert (ec >= 0);
> +      if (ec != 0)
> +	  __waitpid (new_pid, NULL, 0);
>      }
>    else
>      ec = -new_pid;
>  
>    __munmap (stack, stack_size);
>  
> -  close_not_cancel (args.pipe[0]);
> -
>    if ((ec == 0) && (pid != NULL))
>      *pid = new_pid;
>  
>