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

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

I also fixed BZ#17490.  Although POSIX pthread_atfork [1] description only
list 'fork' as the function where should issue the atfork handlers and 
popen description [2] 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
issue for popen:

libc/system	| run atfork handles   | notes 
----------------|----------------------|---------------------------------------	
freebsd	master  |        no            | uses vfork
solaris 11	|        no            |
MacOSX (11.13)  |        no            | implemented through posix_spawn syscall
----------------|----------------------|----------------------------------------

And I also agree that, as for 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 inconsistent are not really required and in some cases
might be unsafe.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/
[2] http://pubs.opengroup.org/onlinepubs/9699919799/

On 17/10/2018 14:11, Adhemerval Zanella wrote:
> 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
>>