Re: [PATCH 2/3] posix: Use posix_spawn on popen

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

Ping.

On 15/09/2018 12:16, Adhemerval Zanella wrote:
> This patch uses posix_spawn on popen instead of fork and execl.  On Linux
> this has the advantage of much lower memory consumption (usually 32 Kb
> minimum for the mmap stack area).
> 
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> 
> 	* libio/iopopen.c (_IO_new_proc_open): use posix_spawn instead of
> 	fork and execl.
> ---
>  ChangeLog       |  3 ++
>  libio/iopopen.c | 97 +++++++++++++++++++++++++++++--------------------
>  2 files changed, 61 insertions(+), 39 deletions(-)
> 
> diff --git a/libio/iopopen.c b/libio/iopopen.c
> index 2eff45b4c8..3cce2e5596 100644
> --- a/libio/iopopen.c
> +++ b/libio/iopopen.c
> @@ -34,7 +34,8 @@
>  #include <not-cancel.h>
>  #include <sys/types.h>
>  #include <sys/wait.h>
> -#include <kernel-features.h>
> +#include <spawn.h>
> +#include <paths.h>
>  
>  struct _IO_proc_file
>  {
> @@ -63,9 +64,8 @@ FILE *
>  _IO_new_proc_open (FILE *fp, const char *command, const char *mode)
>  {
>    int read_or_write;
> -  int parent_end, child_end;
>    int pipe_fds[2];
> -  pid_t child_pid;
> +  int op;
>  
>    int do_read = 0;
>    int do_write = 0;
> @@ -108,59 +108,78 @@ _IO_new_proc_open (FILE *fp, const char *command, const char *mode)
>  
>    if (do_read)
>      {
> -      parent_end = pipe_fds[0];
> -      child_end = pipe_fds[1];
> +      op = 0;
>        read_or_write = _IO_NO_WRITES;
>      }
>    else
>      {
> -      parent_end = pipe_fds[1];
> -      child_end = pipe_fds[0];
> +      op = 1;
>        read_or_write = _IO_NO_READS;
>      }
>  
> -  ((_IO_proc_file *) fp)->pid = child_pid = __fork ();
> -  if (child_pid == 0)
> -    {
> -      int child_std_end = do_read ? 1 : 0;
> -      struct _IO_proc_file *p;
> -
> -      if (child_end != child_std_end)
> -	__dup2 (child_end, child_std_end);
> -      else
> -	/* The descriptor is already the one we will use.  But it must
> -	   not be marked close-on-exec.  Undo the effects.  */
> -	__fcntl (child_end, F_SETFD, 0);
> -      /* POSIX.2:  "popen() shall ensure that any streams from previous
> -         popen() calls that remain open in the parent process are closed
> -	 in the new child process." */
> -      for (p = proc_file_chain; p; p = p->next)
> -	{
> -	  int fd = _IO_fileno ((FILE *) p);
> +  {
> +    posix_spawn_file_actions_t fa;
> +    /* posix_spawn_file_actions_init does not fail.  */
> +    __posix_spawn_file_actions_init (&fa);
>  
> -	  /* If any stream from previous popen() calls has fileno
> -	     child_std_end, it has been already closed by the dup2 syscall
> -	     above.  */
> -	  if (fd != child_std_end)
> -	    __close_nocancel (fd);
> -	}
> +    /* The descriptor is already in the one the child will use.  In this case
> +       it must be moved to another one, otherwise there is no safe way to
> +       remove the close-on-exec flag in the child without creating a FD leak
> +       race in the parent.  */
> +    if (pipe_fds[1 - op] == 1 - op)
> +      {
> +	int tmp = __fcntl (1 - op, F_DUPFD_CLOEXEC, 0);
> +	if (tmp < 0)
> +	  goto spawn_failure;
> +	__close_nocancel (pipe_fds[1 - op]);
> +	pipe_fds[1 - op] = tmp;
> +      }
>  
> -      execl ("/bin/sh", "sh", "-c", command, (char *) 0);
> -      _exit (127);
> -    }
> -  __close_nocancel (child_end);
> -  if (child_pid < 0)
> +    if (__posix_spawn_file_actions_adddup2 (&fa, pipe_fds[1 - op], 1 - op)
> +	!= 0)
> +      goto spawn_failure;
> +
> +    /* POSIX.2: "popen() shall ensure that any streams from previous popen()
> +       calls that remain open in the parent process are closed in the new
> +       child process." */
> +    for (struct _IO_proc_file *p = proc_file_chain; p; p = p->next)
> +      {
> +	int fd = _IO_fileno ((FILE *) p);
> +
> +	/* If any stream from previous popen() calls has fileno
> +	   child_send, it has been already closed by the dup2 syscall
> +	   above.  */
> +	if (fd != 1 - op
> +	    && __posix_spawn_file_actions_addclose (&fa, fd) != 0)
> +	  goto spawn_failure;
> +      }
> +
> +    if (__posix_spawn (&((_IO_proc_file *) fp)->pid, _PATH_BSHELL, &fa, 0,
> +		     (char *const[]){ (char*) "sh", (char*) "-c",
> +		     (char *) command, NULL }, __environ) != 0)
> +      {
> +      spawn_failure:
> +	__posix_spawn_file_actions_destroy (&fa);
> +	__close_nocancel (pipe_fds[1 - op]);
> +	__set_errno (ENOMEM);
> +	return NULL;
> +      }
> +
> +    __posix_spawn_file_actions_destroy (&fa);
> +  }
> +  __close_nocancel (pipe_fds[1 - op]);
> +  if (((_IO_proc_file *) fp)->pid < 0)
>      {
> -      __close_nocancel (parent_end);
> +      __close_nocancel (pipe_fds[op]);
>        return NULL;
>      }
>  
>    if (!do_cloexec)
>      /* Undo the effects of the pipe2 call which set the
>         close-on-exec flag.  */
> -    __fcntl (parent_end, F_SETFD, 0);
> +    __fcntl (pipe_fds[op], F_SETFD, 0);
>  
> -  _IO_fileno (fp) = parent_end;
> +  _IO_fileno (fp) = pipe_fds[op];
>  
>    /* Link into proc_file_chain. */
>  #ifdef _IO_MTSAFE_IO
>