[PATCH v3 1/2] posix: Use posix_spawn on popen

[Adhemerval Zanella]

Ping.

On 25/10/2018 14:41, 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).
> 
> Two issues are also fixed with this change:
> 
>   * BZ#17490: although POSIX pthread_atfork description only list 'fork'
>     as the function that should execute the atfork handlers, popen
>     description states that:
> 
>       '[...] shall be *as if* a child process were created within the popen()
>        call using the fork() function [...]'
> 
>     Other libc/system seems to follow the idea atfork handlers should not be
>     executed for popen:
> 
>     libc/system	| run atfork handles   | notes
>     ------------|----------------------|---------------------------------------
>     Freebsd	|        no            | uses vfork
>     Solaris 11	|        no            |
>     MacOSX 11   |        no            | implemented through posix_spawn syscall
>     ------------|----------------------|----------------------------------------
> 
>     Similar to posix_spawn and system, popen idea is to spawn a different
>     binary so all the POSIX rationale to run the atfork handlers to avoid
>     internal process inconsistency is not really required and in some cases
>     might be unsafe.
> 
>   * BZ#22834: now that proc_file_chain is not copied on another process, it
>     just require to access is through the proc_file_chain_lock.
> 
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> 
> 	[BZ #22834]
> 	[BZ #17490]
> 	* NEWS: Add new semantic for atfork with popen and system.
> 	* libio/iopopen.c (_IO_new_proc_open): use posix_spawn instead of
> 	fork and execl.
> ---
>  ChangeLog       |   6 +++
>  NEWS            |   6 +++
>  libio/iopopen.c | 116 +++++++++++++++++++++++++++++++-----------------
>  3 files changed, 87 insertions(+), 41 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index f054dc0433..c76813d12c 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -30,6 +30,12 @@ Major new features:
>    HTM state is saved and restore lazily (the state being saved even when the
>    process actually does not use HTM).
>  
> +* The popen and system do not run atfork handlers anymore (BZ#17490).
> +  Although it is a possible POSIX violation, the POSIX rationale in
> +  pthread_atfork documentation regarding atfork handlers is to handle
> +  incosistent mutex state after fork call in multithread environment.
> +  In both popen and system there is no direct access to user-defined mutexes.
> +
>  Deprecated and removed features, and other changes affecting compatibility:
>  
>  * The glibc.tune tunable namespace has been renamed to glibc.cpu and the
> diff --git a/libio/iopopen.c b/libio/iopopen.c
> index 2eff45b4c8..da24e60eef 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
>  {
> @@ -64,8 +65,8 @@ _IO_new_proc_open (FILE *fp, const char *command, const char *mode)
>  {
>    int read_or_write;
>    int parent_end, child_end;
> +  int child_std_end;
>    int pipe_fds[2];
> -  pid_t child_pid;
>  
>    int do_read = 0;
>    int do_write = 0;
> @@ -108,59 +109,92 @@ _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];
> +      parent_end = 0;
> +      child_end = 1;
>        read_or_write = _IO_NO_WRITES;
> +      child_std_end = 1;
>      }
>    else
>      {
> -      parent_end = pipe_fds[1];
> -      child_end = pipe_fds[0];
> +      parent_end = 1;
> +      child_end = 0;
>        read_or_write = _IO_NO_READS;
> +      child_std_end = 0;
>      }
>  
> -  ((_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 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[child_end] == child_std_end)
> +      {
> +	int tmp = __fcntl (child_std_end, F_DUPFD_CLOEXEC, 0);
> +	if (tmp < 0)
> +	  goto spawn_failure;
> +	__close_nocancel (pipe_fds[child_end]);
> +	pipe_fds[child_end] = tmp;
> +      }
>  
> -      execl ("/bin/sh", "sh", "-c", command, (char *) 0);
> -      _exit (127);
> -    }
> -  __close_nocancel (child_end);
> -  if (child_pid < 0)
> -    {
> -      __close_nocancel (parent_end);
> -      return NULL;
> -    }
> +    if (__posix_spawn_file_actions_adddup2 (&fa, pipe_fds[child_end],
> +	child_std_end) != 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." */
> +    bool addclose_failure = false;
> +#ifdef _IO_MTSAFE_IO
> +    _IO_cleanup_region_start_noarg (unlock);
> +    _IO_lock_lock (proc_file_chain_lock);
> +#endif
> +    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 != child_std_end
> +	    && __posix_spawn_file_actions_addclose (&fa, fd) != 0)
> +	  {
> +	    addclose_failure = true;
> +	    break;
> +	  }
> +      }
> +#ifdef _IO_MTSAFE_IO
> +     _IO_lock_unlock (proc_file_chain_lock);
> +     _IO_cleanup_region_end (0);
> +#endif
> +    if (addclose_failure)
> +      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[child_end]);
> +	__close_nocancel (pipe_fds[parent_end]);
> +	__set_errno (ENOMEM);
> +	return NULL;
> +      }
> +
> +    __posix_spawn_file_actions_destroy (&fa);
> +  }
> +  __close_nocancel (pipe_fds[child_end]);
>  
>    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[parent_end], F_SETFD, 0);
>  
> -  _IO_fileno (fp) = parent_end;
> +  _IO_fileno (fp) = pipe_fds[parent_end];
>  
>    /* Link into proc_file_chain. */
>  #ifdef _IO_MTSAFE_IO
>