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

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

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
-- 
2.17.1