c5d972
commit 14d0e87d9b8caaa2eca7ca81f1189596671fe4fb
c5d972
Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
c5d972
Date:   Wed Sep 12 10:32:05 2018 -0300
c5d972
c5d972
    posix: Use posix_spawn on popen
c5d972
    
c5d972
    This patch uses posix_spawn on popen instead of fork and execl.  On Linux
c5d972
    this has the advantage of much lower memory consumption (usually 32 Kb
c5d972
    minimum for the mmap stack area).
c5d972
    
c5d972
    Two issues are also fixed with this change:
c5d972
    
c5d972
      * BZ#17490: although POSIX pthread_atfork description only list 'fork'
c5d972
        as the function that should execute the atfork handlers, popen
c5d972
        description states that:
c5d972
    
c5d972
          '[...] shall be *as if* a child process were created within the popen()
c5d972
           call using the fork() function [...]'
c5d972
    
c5d972
        Other libc/system seems to follow the idea atfork handlers should not be
c5d972
        executed for popen:
c5d972
    
c5d972
        libc/system | run atfork handles   | notes
c5d972
        ------------|----------------------|---------------------------------------
c5d972
        Freebsd     |        no            | uses vfork
c5d972
        Solaris 11  |        no            |
c5d972
        MacOSX 11   |        no            | implemented through posix_spawn syscall
c5d972
        ------------|----------------------|----------------------------------------
c5d972
    
c5d972
        Similar to posix_spawn and system, popen idea is to spawn a different
c5d972
        binary so all the POSIX rationale to run the atfork handlers to avoid
c5d972
        internal process inconsistency is not really required and in some cases
c5d972
        might be unsafe.
c5d972
    
c5d972
      * BZ#22834: the described scenario, where the forked process might access
c5d972
        invalid memory due an inconsistent state in multithreaded environment,
c5d972
        should not happen because posix_spawn does not access the affected
c5d972
        data structure (proc_file_chain).
c5d972
    
c5d972
    Checked on x86_64-linux-gnu and i686-linux-gnu.
c5d972
    
c5d972
            [BZ #22834]
c5d972
            [BZ #17490]
c5d972
            * NEWS: Add new semantic for atfork with popen and system.
c5d972
            * libio/iopopen.c (_IO_new_proc_open): use posix_spawn instead of
c5d972
            fork and execl.
c5d972
c5d972
diff --git a/libio/iopopen.c b/libio/iopopen.c
c5d972
index 2eff45b4c80b5cd6..c768295180fdf809 100644
c5d972
--- a/libio/iopopen.c
c5d972
+++ b/libio/iopopen.c
c5d972
@@ -34,7 +34,8 @@
c5d972
 #include <not-cancel.h>
c5d972
 #include <sys/types.h>
c5d972
 #include <sys/wait.h>
c5d972
-#include <kernel-features.h>
c5d972
+#include <spawn.h>
c5d972
+#include <paths.h>
c5d972
 
c5d972
 struct _IO_proc_file
c5d972
 {
c5d972
@@ -59,13 +60,60 @@ unlock (void *not_used)
c5d972
 }
c5d972
 #endif
c5d972
 
c5d972
+/* POSIX states popen shall ensure that any streams from previous popen()
c5d972
+   calls that remain open in the parent process should be closed in the new
c5d972
+   child process.
c5d972
+   To avoid a race-condition between checking which file descriptors need to
c5d972
+   be close (by transversing the proc_file_chain list) and the insertion of a
c5d972
+   new one after a successful posix_spawn this function should be called
c5d972
+   with proc_file_chain_lock acquired.  */
c5d972
+static bool
c5d972
+spawn_process (posix_spawn_file_actions_t *fa, FILE *fp, const char *command,
c5d972
+	       int do_cloexec, int pipe_fds[2], int parent_end, int child_end,
c5d972
+	       int child_pipe_fd)
c5d972
+{
c5d972
+
c5d972
+  for (struct _IO_proc_file *p = proc_file_chain; p; p = p->next)
c5d972
+    {
c5d972
+      int fd = _IO_fileno ((FILE *) p);
c5d972
+
c5d972
+      /* If any stream from previous popen() calls has fileno
c5d972
+	 child_pipe_fd, it has been already closed by the adddup2 action
c5d972
+	 above.  */
c5d972
+      if (fd != child_pipe_fd
c5d972
+	  && __posix_spawn_file_actions_addclose (fa, fd) != 0)
c5d972
+	return false;
c5d972
+    }
c5d972
+
c5d972
+  if (__posix_spawn (&((_IO_proc_file *) fp)->pid, _PATH_BSHELL, fa, 0,
c5d972
+		     (char *const[]){ (char*) "sh", (char*) "-c",
c5d972
+		     (char *) command, NULL }, __environ) != 0)
c5d972
+    return false;
c5d972
+
c5d972
+  __close_nocancel (pipe_fds[child_end]);
c5d972
+
c5d972
+  if (!do_cloexec)
c5d972
+    /* Undo the effects of the pipe2 call which set the
c5d972
+       close-on-exec flag.  */
c5d972
+    __fcntl (pipe_fds[parent_end], F_SETFD, 0);
c5d972
+
c5d972
+  _IO_fileno (fp) = pipe_fds[parent_end];
c5d972
+
c5d972
+  ((_IO_proc_file *) fp)->next = proc_file_chain;
c5d972
+  proc_file_chain = (_IO_proc_file *) fp;
c5d972
+
c5d972
+  return true;
c5d972
+}
c5d972
+
c5d972
 FILE *
c5d972
 _IO_new_proc_open (FILE *fp, const char *command, const char *mode)
c5d972
 {
c5d972
   int read_or_write;
c5d972
+  /* These are indexes for pipe_fds.  */
c5d972
   int parent_end, child_end;
c5d972
   int pipe_fds[2];
c5d972
-  pid_t child_pid;
c5d972
+  int child_pipe_fd;
c5d972
+  bool spawn_ok;
c5d972
 
c5d972
   int do_read = 0;
c5d972
   int do_write = 0;
c5d972
@@ -108,72 +156,62 @@ _IO_new_proc_open (FILE *fp, const char *command, const char *mode)
c5d972
 
c5d972
   if (do_read)
c5d972
     {
c5d972
-      parent_end = pipe_fds[0];
c5d972
-      child_end = pipe_fds[1];
c5d972
+      parent_end = 0;
c5d972
+      child_end = 1;
c5d972
       read_or_write = _IO_NO_WRITES;
c5d972
+      child_pipe_fd = 1;
c5d972
     }
c5d972
   else
c5d972
     {
c5d972
-      parent_end = pipe_fds[1];
c5d972
-      child_end = pipe_fds[0];
c5d972
+      parent_end = 1;
c5d972
+      child_end = 0;
c5d972
       read_or_write = _IO_NO_READS;
c5d972
+      child_pipe_fd = 0;
c5d972
     }
c5d972
 
c5d972
-  ((_IO_proc_file *) fp)->pid = child_pid = __fork ();
c5d972
-  if (child_pid == 0)
c5d972
-    {
c5d972
-      int child_std_end = do_read ? 1 : 0;
c5d972
-      struct _IO_proc_file *p;
c5d972
-
c5d972
-      if (child_end != child_std_end)
c5d972
-	__dup2 (child_end, child_std_end);
c5d972
-      else
c5d972
-	/* The descriptor is already the one we will use.  But it must
c5d972
-	   not be marked close-on-exec.  Undo the effects.  */
c5d972
-	__fcntl (child_end, F_SETFD, 0);
c5d972
-      /* POSIX.2:  "popen() shall ensure that any streams from previous
c5d972
-         popen() calls that remain open in the parent process are closed
c5d972
-	 in the new child process." */
c5d972
-      for (p = proc_file_chain; p; p = p->next)
c5d972
-	{
c5d972
-	  int fd = _IO_fileno ((FILE *) p);
c5d972
+  posix_spawn_file_actions_t fa;
c5d972
+  /* posix_spawn_file_actions_init does not fail.  */
c5d972
+  __posix_spawn_file_actions_init (&fa);
c5d972
 
c5d972
-	  /* If any stream from previous popen() calls has fileno
c5d972
-	     child_std_end, it has been already closed by the dup2 syscall
c5d972
-	     above.  */
c5d972
-	  if (fd != child_std_end)
c5d972
-	    __close_nocancel (fd);
c5d972
-	}
c5d972
-
c5d972
-      execl ("/bin/sh", "sh", "-c", command, (char *) 0);
c5d972
-      _exit (127);
c5d972
-    }
c5d972
-  __close_nocancel (child_end);
c5d972
-  if (child_pid < 0)
c5d972
+  /* The descriptor is already the one the child will use.  In this case
c5d972
+     it must be moved to another one otherwise, there is no safe way to
c5d972
+     remove the close-on-exec flag in the child without creating a FD leak
c5d972
+     race in the parent.  */
c5d972
+  if (pipe_fds[child_end] == child_pipe_fd)
c5d972
     {
c5d972
-      __close_nocancel (parent_end);
c5d972
-      return NULL;
c5d972
+      int tmp = __fcntl (child_pipe_fd, F_DUPFD_CLOEXEC, 0);
c5d972
+      if (tmp < 0)
c5d972
+	goto spawn_failure;
c5d972
+      __close_nocancel (pipe_fds[child_end]);
c5d972
+      pipe_fds[child_end] = tmp;
c5d972
     }
c5d972
 
c5d972
-  if (!do_cloexec)
c5d972
-    /* Undo the effects of the pipe2 call which set the
c5d972
-       close-on-exec flag.  */
c5d972
-    __fcntl (parent_end, F_SETFD, 0);
c5d972
+  if (__posix_spawn_file_actions_adddup2 (&fa, pipe_fds[child_end],
c5d972
+      child_pipe_fd) != 0)
c5d972
+    goto spawn_failure;
c5d972
 
c5d972
-  _IO_fileno (fp) = parent_end;
c5d972
-
c5d972
-  /* Link into proc_file_chain. */
c5d972
 #ifdef _IO_MTSAFE_IO
c5d972
   _IO_cleanup_region_start_noarg (unlock);
c5d972
   _IO_lock_lock (proc_file_chain_lock);
c5d972
 #endif
c5d972
-  ((_IO_proc_file *) fp)->next = proc_file_chain;
c5d972
-  proc_file_chain = (_IO_proc_file *) fp;
c5d972
+  spawn_ok = spawn_process (&fa, fp, command, do_cloexec, pipe_fds,
c5d972
+			    parent_end, child_end, child_pipe_fd);
c5d972
 #ifdef _IO_MTSAFE_IO
c5d972
   _IO_lock_unlock (proc_file_chain_lock);
c5d972
   _IO_cleanup_region_end (0);
c5d972
 #endif
c5d972
 
c5d972
+  __posix_spawn_file_actions_destroy (&fa);
c5d972
+
c5d972
+  if (!spawn_ok)
c5d972
+    {
c5d972
+    spawn_failure:
c5d972
+      __close_nocancel (pipe_fds[child_end]);
c5d972
+      __close_nocancel (pipe_fds[parent_end]);
c5d972
+      __set_errno (ENOMEM);
c5d972
+      return NULL;
c5d972
+    }
c5d972
+
c5d972
   _IO_mask_flags (fp, read_or_write, _IO_NO_READS|_IO_NO_WRITES);
c5d972
   return fp;
c5d972
 }