Re: [PATCH 2/3] posix: Improve default posix_spawn implementation

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

Since this is not used in any port currently (since hurd uses its own
implementation which I think suffers from the same issue of the current
posix one) and this patch just align the posix one to Linux recent
fixes I will commit this shortly if no one objects. 

On 15/05/2017 16:36, Adhemerval Zanella wrote:
> This patch improves the default posix implementation of posix_spawn{p}
> and align with Linux one.  The main idea is to try shared common
> implementation bits with Linux and avoid code duplication, fix some
> issues already fixed in Linux code, and deprecated vfork internal
> usage (source of various bug reports).  In a short:
> 
>    - It moves POSIX_SPAWN_USEVFORK usage and sets it a no-op.  Since
>      the process that actually spawn the new process do not share
>      memory with parent (with vfork), it fixes BZ#14750 for this
>      implementation.
> 
>    - It uses a pipe to correctly obtain the return upon failure
>      of execution (BZ#18433).
> 
>    - It correctly enable/disable asynchronous cancellation (checked
>      on ptl/tst-exec5.c).
> 
>    - It correctly disable/enable signal handling.
> 
> Using this version instead of Linux shows only one regression,
> posix/tst-spawn3, because of pipe2 usage which increase total
> number of file descriptor.
> 
> 	* sysdeps/posix/spawni.c (__spawni_child): New function.
> 	(__spawni): Rename to __spawnix.
> ---
>  ChangeLog              |   3 +
>  sysdeps/posix/spawni.c | 362 ++++++++++++++++++++++++-------------------------
>  2 files changed, 182 insertions(+), 183 deletions(-)
> 
> diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c
> index 9cad25c..e096a42 100644
> --- a/sysdeps/posix/spawni.c
> +++ b/sysdeps/posix/spawni.c
> @@ -16,20 +16,23 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> -#include <errno.h>
> +#include <spawn.h>
> +#include <assert.h>
>  #include <fcntl.h>
>  #include <paths.h>
> -#include <spawn.h>
> -#include <stdbool.h>
> -#include <stdlib.h>
>  #include <string.h>
> -#include <unistd.h>
> -#include <signal.h>
>  #include <sys/resource.h>
> -#include "spawn_int.h"
> +#include <sys/wait.h>
> +#include <sys/param.h>
> +#include <sys/mman.h>
>  #include <not-cancel.h>
>  #include <local-setxid.h>
>  #include <shlib-compat.h>
> +#include <nptl/pthreadP.h>
> +#include <dl-sysdep.h>
> +#include <libc-pointer-arith.h>
> +#include <ldsodefs.h>
> +#include "spawn_int.h"
>  
>  
>  /* The Unix standard contains a long explanation of the way to signal
> @@ -39,94 +42,59 @@
>     normal program exit with the exit code 127.  */
>  #define SPAWN_ERROR	127
>  
> -
> -/* The file is accessible but it is not an executable file.  Invoke
> -   the shell to interpret it as a script.  */
> -static void
> -internal_function
> -script_execute (const char *file, char *const argv[], char *const envp[])
> +struct posix_spawn_args
>  {
> -  /* Count the arguments.  */
> -  int argc = 0;
> -  while (argv[argc++])
> -    ;
> -
> -  /* Construct an argument list for the shell.  */
> -  {
> -    char *new_argv[argc + 1];
> -    new_argv[0] = (char *) _PATH_BSHELL;
> -    new_argv[1] = (char *) file;
> -    while (argc > 1)
> -      {
> -	new_argv[argc] = argv[argc - 1];
> -	--argc;
> -      }
> -
> -    /* Execute the shell.  */
> -    __execve (new_argv[0], new_argv, envp);
> -  }
> -}
> -
> -static inline void
> -maybe_script_execute (const char *file, char *const argv[], char *const envp[],
> -                      int xflags)
> +  sigset_t oldmask;
> +  const char *file;
> +  int (*exec) (const char *, char *const *, char *const *);
> +  const posix_spawn_file_actions_t *fa;
> +  const posix_spawnattr_t *restrict attr;
> +  char *const *argv;
> +  ptrdiff_t argc;
> +  char *const *envp;
> +  int xflags;
> +  int pipe[2];
> +};
> +
> +/* Older version requires that shell script without shebang definition
> +   to be called explicitly using /bin/sh (_PATH_BSHELL).  */
> +static void
> +maybe_script_execute (struct posix_spawn_args *args)
>  {
>    if (SHLIB_COMPAT (libc, GLIBC_2_2, GLIBC_2_15)
> -      && (xflags & SPAWN_XFLAGS_TRY_SHELL)
> -      && errno == ENOEXEC)
> -    script_execute (file, argv, envp);
> -}
> -
> -/* Spawn a new process executing PATH with the attributes describes in *ATTRP.
> -   Before running the process perform the actions described in FILE-ACTIONS. */
> -int
> -__spawni (pid_t *pid, const char *file,
> -	  const posix_spawn_file_actions_t *file_actions,
> -	  const posix_spawnattr_t *attrp, char *const argv[],
> -	  char *const envp[], int xflags)
> -{
> -  pid_t new_pid;
> -  char *path, *p, *name;
> -  size_t len;
> -  size_t pathlen;
> -
> -  /* Do this once.  */
> -  short int flags = attrp == NULL ? 0 : attrp->__flags;
> -
> -  /* Generate the new process.  */
> -  if ((flags & POSIX_SPAWN_USEVFORK) != 0
> -      /* If no major work is done, allow using vfork.  Note that we
> -	 might perform the path searching.  But this would be done by
> -	 a call to execvp(), too, and such a call must be OK according
> -	 to POSIX.  */
> -      || ((flags & (POSIX_SPAWN_SETSIGMASK | POSIX_SPAWN_SETSIGDEF
> -		    | POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER
> -		    | POSIX_SPAWN_SETPGROUP | POSIX_SPAWN_RESETIDS
> -		    | POSIX_SPAWN_SETSID)) == 0
> -	  && file_actions == NULL))
> -    new_pid = __vfork ();
> -  else
> -    new_pid = __fork ();
> -
> -  if (new_pid != 0)
> +      && (args->xflags & SPAWN_XFLAGS_TRY_SHELL) && errno == ENOEXEC)
>      {
> -      if (new_pid < 0)
> -	return errno;
> -
> -      /* The call was successful.  Store the PID if necessary.  */
> -      if (pid != NULL)
> -	*pid = new_pid;
> +      char *const *argv = args->argv;
> +      ptrdiff_t argc = args->argc;
> +
> +      /* Construct an argument list for the shell.  */
> +      char *new_argv[argc + 1];
> +      new_argv[0] = (char *) _PATH_BSHELL;
> +      new_argv[1] = (char *) args->file;
> +      if (argc > 1)
> +	memcpy (new_argv + 2, argv + 1, argc * sizeof(char *));
> +      else
> +	new_argv[2] = NULL;
>  
> -      return 0;
> +      /* Execute the shell.  */
> +      args->exec (new_argv[0], new_argv, args->envp);
>      }
> +}
> +
> +/* Function used in the clone call to setup the signals mask, posix_spawn
> +   attributes, and file actions.  */
> +static int
> +__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 ret;
>  
> -  /* Set signal mask.  */
> -  if ((flags & POSIX_SPAWN_SETSIGMASK) != 0
> -      && __sigprocmask (SIG_SETMASK, &attrp->__ss, NULL) != 0)
> -    _exit (SPAWN_ERROR);
> +  __close (args->pipe[0]);
>  
>    /* Set signal default action.  */
> -  if ((flags & POSIX_SPAWN_SETSIGDEF) != 0)
> +  if ((attr->__flags & POSIX_SPAWN_SETSIGDEF) != 0)
>      {
>        /* We have to iterate over all signals.  This could possibly be
>  	 done better but it requires system specific solutions since
> @@ -139,41 +107,41 @@ __spawni (pid_t *pid, const char *file,
>        sa.sa_handler = SIG_DFL;
>  
>        for (sig = 1; sig <= _NSIG; ++sig)
> -	if (__sigismember (&attrp->__sd, sig) != 0
> +	if (__sigismember (&attr->__sd, sig) != 0
>  	    && __sigaction (sig, &sa, NULL) != 0)
> -	  _exit (SPAWN_ERROR);
> -
> +	  goto fail;
>      }
>  
>  #ifdef _POSIX_PRIORITY_SCHEDULING
>    /* Set the scheduling algorithm and parameters.  */
> -  if ((flags & (POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER))
> +  if ((attr->__flags & (POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER))
>        == POSIX_SPAWN_SETSCHEDPARAM)
>      {
> -      if (__sched_setparam (0, &attrp->__sp) == -1)
> -	_exit (SPAWN_ERROR);
> +      if ((ret = __sched_setparam (0, &attr->__sp)) == -1)
> +	goto fail;
>      }
> -  else if ((flags & POSIX_SPAWN_SETSCHEDULER) != 0)
> +  else if ((attr->__flags & POSIX_SPAWN_SETSCHEDULER) != 0)
>      {
> -      if (__sched_setscheduler (0, attrp->__policy, &attrp->__sp) == -1)
> -	_exit (SPAWN_ERROR);
> +      if ((ret = __sched_setscheduler (0, attr->__policy, &attr->__sp) == -1))
> +	goto fail;
>      }
>  #endif
>  
> -  if ((flags & POSIX_SPAWN_SETSID) != 0
> -      && __setsid () < 0)
> -    _exit (SPAWN_ERROR);
> +  /* Set the process session ID.  */
> +  if ((attr->__flags & POSIX_SPAWN_SETSID) != 0
> +      && (ret = __setsid ()) < 0)
> +    goto fail;
>  
>    /* Set the process group ID.  */
> -  if ((flags & POSIX_SPAWN_SETPGROUP) != 0
> -      && __setpgid (0, attrp->__pgrp) != 0)
> -    _exit (SPAWN_ERROR);
> +  if ((attr->__flags & POSIX_SPAWN_SETPGROUP) != 0
> +      && (ret =__setpgid (0, attr->__pgrp)) != 0)
> +    goto fail;
>  
>    /* Set the effective user and group IDs.  */
> -  if ((flags & POSIX_SPAWN_RESETIDS) != 0
> -      && (local_seteuid (__getuid ()) != 0
> -	  || local_setegid (__getgid ()) != 0))
> -    _exit (SPAWN_ERROR);
> +  if ((attr->__flags & POSIX_SPAWN_RESETIDS) != 0
> +      && ((ret = local_seteuid (__getuid ())) != 0
> +	  || (ret = local_setegid (__getgid ())) != 0))
> +    goto fail;
>  
>    /* Execute the file actions.  */
>    if (file_actions != NULL)
> @@ -191,7 +159,7 @@ __spawni (pid_t *pid, const char *file,
>  	    case spawn_do_close:
>  	      if (close_not_cancel (action->action.close_action.fd) != 0)
>  		{
> -		  if (! have_fdlimit)
> +		  if (have_fdlimit == 0)
>  		    {
>  		      __getrlimit64 (RLIMIT_NOFILE, &fdlimit);
>  		      have_fdlimit = true;
> @@ -200,120 +168,148 @@ __spawni (pid_t *pid, const char *file,
>  		  /* Only signal errors for file descriptors out of range.  */
>  		  if (action->action.close_action.fd < 0
>  		      || action->action.close_action.fd >= fdlimit.rlim_cur)
> -		    /* Signal the error.  */
> -		    _exit (SPAWN_ERROR);
> +		    {
> +		      ret = -1;
> +		      goto fail;
> +		    }
>  		}
>  	      break;
>  
>  	    case spawn_do_open:
>  	      {
> +		/* POSIX states that if fildes was already an open file descriptor,
> +		   it shall be closed before the new file is opened.  This avoid
> +		   pontential issues when posix_spawn plus addopen action is called
> +		   with the process already at maximum number of file descriptor
> +		   opened and also for multiple actions on single-open special
> +		   paths (like /dev/watchdog).  */
> +		close_not_cancel (action->action.open_action.fd);
> +
>  		int new_fd = open_not_cancel (action->action.open_action.path,
>  					      action->action.open_action.oflag
>  					      | O_LARGEFILE,
>  					      action->action.open_action.mode);
>  
> -		if (new_fd == -1)
> -		  /* The `open' call failed.  */
> -		  _exit (SPAWN_ERROR);
> +		if ((ret = new_fd) == -1)
> +		  goto fail;
>  
>  		/* Make sure the desired file descriptor is used.  */
>  		if (new_fd != action->action.open_action.fd)
>  		  {
> -		    if (__dup2 (new_fd, action->action.open_action.fd)
> +		    if ((ret = __dup2 (new_fd, action->action.open_action.fd))
>  			!= action->action.open_action.fd)
> -		      /* The `dup2' call failed.  */
> -		      _exit (SPAWN_ERROR);
> +		      goto fail;
>  
> -		    if (close_not_cancel (new_fd) != 0)
> -		      /* The `close' call failed.  */
> -		      _exit (SPAWN_ERROR);
> +		    if ((ret = close_not_cancel (new_fd) != 0))
> +		      goto fail;
>  		  }
>  	      }
>  	      break;
>  
>  	    case spawn_do_dup2:
> -	      if (__dup2 (action->action.dup2_action.fd,
> -			  action->action.dup2_action.newfd)
> +	      if ((ret = __dup2 (action->action.dup2_action.fd,
> +			  	 action->action.dup2_action.newfd))
>  		  != action->action.dup2_action.newfd)
> -		/* The `dup2' call failed.  */
> -		_exit (SPAWN_ERROR);
> +		goto fail;
>  	      break;
>  	    }
>  	}
>      }
>  
> -  if ((xflags & SPAWN_XFLAGS_USE_PATH) == 0 || strchr (file, '/') != NULL)
> -    {
> -      /* The FILE parameter is actually a path.  */
> -      __execve (file, argv, envp);
> +  /* Set the initial signal mask of the child if POSIX_SPAWN_SETSIGMASK
> +     is set, otherwise restore the previous one.  */
> +  __sigprocmask (SIG_SETMASK, (attr->__flags & POSIX_SPAWN_SETSIGMASK)
> +		 ? &attr->__ss : &args->oldmask, 0);
>  
> -      maybe_script_execute (file, argv, envp, xflags);
> +  args->exec (args->file, args->argv, args->envp);
>  
> -      /* Oh, oh.  `execve' returns.  This is bad.  */
> -      _exit (SPAWN_ERROR);
> -    }
> +  /* This is compatibility function required to enable posix_spawn run
> +     script without shebang definition for older posix_spawn versions
> +     (2.15).  */
> +  maybe_script_execute (args);
>  
> -  /* We have to search for FILE on the path.  */
> -  path = getenv ("PATH");
> -  if (path == NULL)
> -    {
> -      /* There is no `PATH' in the environment.
> -	 The default search path is the current directory
> -	 followed by the path `confstr' returns for `_CS_PATH'.  */
> -      len = confstr (_CS_PATH, (char *) NULL, 0);
> -      path = (char *) __alloca (1 + len);
> -      path[0] = ':';
> -      (void) confstr (_CS_PATH, path + 1, len);
> -    }
> +  ret = -errno;
>  
> -  len = strlen (file) + 1;
> -  pathlen = strlen (path);
> -  name = __alloca (pathlen + len + 1);
> -  /* Copy the file name at the top.  */
> -  name = (char *) memcpy (name + pathlen + 1, file, len);
> -  /* And add the slash.  */
> -  *--name = '/';
> +fail:
> +  /* Since sizeof errno < PIPE_BUF, the write is atomic. */
> +  ret = -ret;
> +  if (ret)
> +    while (write_not_cancel (args->pipe[1], &ret, sizeof (ret)) < 0);
>  
> -  p = path;
> -  do
> -    {
> -      char *startp;
> +  _exit (SPAWN_ERROR);
> +}
>  
> -      path = p;
> -      p = __strchrnul (path, ':');
> +/* Spawn a new process executing PATH with the attributes describes in *ATTRP.
> +   Before running the process perform the actions described in FILE-ACTIONS. */
> +int
> +__spawnix (pid_t *pid, const char *file,
> +	   const posix_spawn_file_actions_t *file_actions,
> +	   const posix_spawnattr_t *attrp, char *const argv[],
> +	   char *const envp[], int xflags,
> +	   int (*exec) (const char *, char *const *, char *const *))
> +{
> +  struct posix_spawn_args args;
> +  int ec;
>  
> -      if (p == path)
> -	/* Two adjacent colons, or a colon at the beginning or the end
> -	   of `PATH' means to search the current directory.  */
> -	startp = name + 1;
> -      else
> -	startp = (char *) memcpy (name - (p - path), path, p - path);
> +  if (__pipe2 (args.pipe, O_CLOEXEC))
> +    return errno;
>  
> -      /* Try to execute this name.  If it works, execv will not return.  */
> -      __execve (startp, argv, envp);
> +  /* Disable asynchronous cancellation.  */
> +  int state;
> +  __libc_ptf_call (__pthread_setcancelstate,
> +                   (PTHREAD_CANCEL_DISABLE, &state), 0);
>  
> -      maybe_script_execute (startp, argv, envp, xflags);
> +  ptrdiff_t argc = 0;
> +  ptrdiff_t limit = INT_MAX - 1;
> +  while (argv[argc++] != NULL)
> +    if (argc == limit)
> +      {
> +	errno = E2BIG;
> +	return errno;
> +      }
>  
> -      switch (errno)
> -	{
> -	case EACCES:
> -	case ENOENT:
> -	case ESTALE:
> -	case ENOTDIR:
> -	  /* Those errors indicate the file is missing or not executable
> -	     by us, in which case we want to just try the next path
> -	     directory.  */
> -	  break;
> -
> -	default:
> -	  /* Some other error means we found an executable file, but
> -	     something went wrong executing it; return the error to our
> -	     caller.  */
> -	  _exit (SPAWN_ERROR);
> -	    }
> +  args.file = file;
> +  args.exec = exec;
> +  args.fa = file_actions;
> +  args.attr = attrp ? attrp : &(const posix_spawnattr_t) { 0 };
> +  args.argv = argv;
> +  args.argc = argc;
> +  args.envp = envp;
> +  args.xflags = xflags;
> +
> +  /* Generate the new process.  */
> +  pid_t new_pid = __fork ();
> +
> +  if (new_pid == 0)
> +    __spawni_child (&args);
> +  else if (new_pid > 0)
> +    {
> +      __close (args.pipe[1]);
> +
> +      if (__read (args.pipe[0], &ec, sizeof ec) != sizeof ec)
> +	ec = 0;
> +      else
> +	__waitpid (new_pid, &(int) { 0 }, 0);
>      }
> -  while (*p++ != '\0');
> +  else
> +    ec = -new_pid;
>  
> -  /* Return with an error.  */
> -  _exit (SPAWN_ERROR);
> +  __close (args.pipe[0]);
> +
> +  if ((ec == 0) && (pid != NULL))
> +    *pid = new_pid;
> +
> +  __libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0);
> +
> +  return ec;
> +}
> +
> +int
> +__spawni (pid_t * pid, const char *file,
> +	  const posix_spawn_file_actions_t * acts,
> +	  const posix_spawnattr_t * attrp, char *const argv[],
> +	  char *const envp[], int xflags)
> +{
> +  return __spawnix (pid, file, acts, attrp, argv, envp, xflags,
> +		    xflags & SPAWN_XFLAGS_USE_PATH ? __execvpe : __execve);
>  }
>