[glib: 9/15] gsubprocess: Use new source/target FD mapping functionality in g_spawn()

[Philip Withnall <pwithnall src gnome org>]

commit 67a589e505311de4dc244f69673c129d15c9880f
Author: Philip Withnall <pwithnall endlessos org>
Date:   Tue Oct 13 13:40:37 2020 +0100

    gsubprocess: Use new source/target FD mapping functionality in g_spawn()
    
    This improves performance by eliminating the use of a
    `GSpawnChildSetupFunc` in the common case (since that setup code has now
    moved into `g_spawn*()` itself), and enables the use of the fix to avoid
    the child error reporting FD being overwritten by target FD mappings,
    introduced via `g_spawn_async_with_pipes_and_fds()`.
    
    It reworks how the source/target FD mapping is stored within
    `GSubprocessLauncher` to match what `g_spawn*()` uses. The two
    approaches are equivalent.
    
    Signed-off-by: Philip Withnall <pwithnall endlessos org>
    
    Fixes: #2097

 gio/gsubprocess.c                 | 220 ++++++--------------------------------
 gio/gsubprocesslauncher-private.h |   4 +-
 gio/gsubprocesslauncher.c         |  39 +++----
 3 files changed, 48 insertions(+), 215 deletions(-)
---
diff --git a/gio/gsubprocess.c b/gio/gsubprocess.c
index 665d729ed..77a23efc3 100644
--- a/gio/gsubprocess.c
+++ b/gio/gsubprocess.c
@@ -179,160 +179,6 @@ enum
   N_PROPS
 };
 
-#ifdef G_OS_UNIX
-typedef struct
-{
-  gint                 fds[3];
-  GSpawnChildSetupFunc child_setup_func;
-  gpointer             child_setup_data;
-  GArray              *basic_fd_assignments;
-  GArray              *needdup_fd_assignments;
-} ChildData;
-
-static void
-unset_cloexec (int fd)
-{
-  int flags;
-  int result;
-
-  flags = fcntl (fd, F_GETFD, 0);
-
-  if (flags != -1)
-    {
-      int errsv;
-      flags &= (~FD_CLOEXEC);
-      do
-        {
-          result = fcntl (fd, F_SETFD, flags);
-          errsv = errno;
-        }
-      while (result == -1 && errsv == EINTR);
-    }
-}
-
-static int
-dupfd_cloexec (int parent_fd)
-{
-  int fd, errsv;
-#ifdef F_DUPFD_CLOEXEC
-  do
-    {
-      fd = fcntl (parent_fd, F_DUPFD_CLOEXEC, 3);
-      errsv = errno;
-    }
-  while (fd == -1 && errsv == EINTR);
-#else
-  /* OS X Snow Lion and earlier don't have F_DUPFD_CLOEXEC:
-   * https://bugzilla.gnome.org/show_bug.cgi?id=710962
-   */
-  int result, flags;
-  do
-    {
-      fd = fcntl (parent_fd, F_DUPFD, 3);
-      errsv = errno;
-    }
-  while (fd == -1 && errsv == EINTR);
-  flags = fcntl (fd, F_GETFD, 0);
-  if (flags != -1)
-    {
-      flags |= FD_CLOEXEC;
-      do
-        {
-          result = fcntl (fd, F_SETFD, flags);
-          errsv = errno;
-        }
-      while (result == -1 && errsv == EINTR);
-    }
-#endif
-  return fd;
-}
-
-/*
- * Based on code derived from
- * gnome-terminal:src/terminal-screen.c:terminal_screen_child_setup(),
- * used under the LGPLv2+ with permission from author.
- */
-static void
-child_setup (gpointer user_data)
-{
-  ChildData *child_data = user_data;
-  guint i;
-  gint result;
-  int errsv;
-
-  /* We're on the child side now.  "Rename" the file descriptors in
-   * child_data.fds[] to stdin/stdout/stderr.
-   *
-   * We don't close the originals.  It's possible that the originals
-   * should not be closed and if they should be closed then they should
-   * have been created O_CLOEXEC.
-   */
-  for (i = 0; i < 3; i++)
-    if (child_data->fds[i] != -1 && child_data->fds[i] != (gint) i)
-      {
-        do
-          {
-            result = dup2 (child_data->fds[i], i);
-            errsv = errno;
-          }
-        while (result == -1 && errsv == EINTR);
-      }
-
-  /* Basic fd assignments we can just unset FD_CLOEXEC */
-  if (child_data->basic_fd_assignments)
-    {
-      for (i = 0; i < child_data->basic_fd_assignments->len; i++)
-        {
-          gint fd = g_array_index (child_data->basic_fd_assignments, int, i);
-
-          unset_cloexec (fd);
-        }
-    }
-
-  /* If we're doing remapping fd assignments, we need to handle
-   * the case where the user has specified e.g.:
-   * 5 -> 4, 4 -> 6
-   *
-   * We do this by duping the source fds temporarily.
-   */ 
-  if (child_data->needdup_fd_assignments)
-    {
-      for (i = 0; i < child_data->needdup_fd_assignments->len; i += 2)
-        {
-          gint parent_fd = g_array_index (child_data->needdup_fd_assignments, int, i);
-          gint new_parent_fd;
-
-          new_parent_fd = dupfd_cloexec (parent_fd);
-
-          g_array_index (child_data->needdup_fd_assignments, int, i) = new_parent_fd;
-        }
-      for (i = 0; i < child_data->needdup_fd_assignments->len; i += 2)
-        {
-          gint parent_fd = g_array_index (child_data->needdup_fd_assignments, int, i);
-          gint child_fd = g_array_index (child_data->needdup_fd_assignments, int, i+1);
-
-          if (parent_fd == child_fd)
-            {
-              unset_cloexec (parent_fd);
-            }
-          else
-            {
-              do
-                {
-                  result = dup2 (parent_fd, child_fd);
-                  errsv = errno;
-                }
-              while (result == -1 && errsv == EINTR);
-              (void) close (parent_fd);
-            }
-        }
-    }
-
-  if (child_data->child_setup_func)
-    child_data->child_setup_func (child_data->child_setup_data);
-}
-#endif
-
 static GInputStream *
 platform_input_stream_from_spawn_fd (gint fd)
 {
@@ -450,12 +296,12 @@ initable_init (GInitable     *initable,
                GError       **error)
 {
   GSubprocess *self = G_SUBPROCESS (initable);
-#ifdef G_OS_UNIX
-  ChildData child_data = { { -1, -1, -1 }, 0, NULL, NULL, NULL };
-#endif
   gint *pipe_ptrs[3] = { NULL, NULL, NULL };
   gint pipe_fds[3] = { -1, -1, -1 };
   gint close_fds[3] = { -1, -1, -1 };
+#ifdef G_OS_UNIX
+  gint stdin_fd = -1, stdout_fd = -1, stderr_fd = -1;
+#endif
   GSpawnFlags spawn_flags = 0;
   gboolean success = FALSE;
   gint i;
@@ -480,11 +326,11 @@ initable_init (GInitable     *initable,
   else if (self->launcher)
     {
       if (self->launcher->stdin_fd != -1)
-        child_data.fds[0] = self->launcher->stdin_fd;
+        stdin_fd = self->launcher->stdin_fd;
       else if (self->launcher->stdin_path != NULL)
         {
-          child_data.fds[0] = close_fds[0] = unix_open_file (self->launcher->stdin_path, O_RDONLY, error);
-          if (child_data.fds[0] == -1)
+          stdin_fd = close_fds[0] = unix_open_file (self->launcher->stdin_path, O_RDONLY, error);
+          if (stdin_fd == -1)
             goto out;
         }
     }
@@ -499,11 +345,11 @@ initable_init (GInitable     *initable,
   else if (self->launcher)
     {
       if (self->launcher->stdout_fd != -1)
-        child_data.fds[1] = self->launcher->stdout_fd;
+        stdout_fd = self->launcher->stdout_fd;
       else if (self->launcher->stdout_path != NULL)
         {
-          child_data.fds[1] = close_fds[1] = unix_open_file (self->launcher->stdout_path, O_CREAT | 
O_WRONLY, error);
-          if (child_data.fds[1] == -1)
+          stdout_fd = close_fds[1] = unix_open_file (self->launcher->stdout_path, O_CREAT | O_WRONLY, error);
+          if (stdout_fd == -1)
             goto out;
         }
     }
@@ -516,29 +362,21 @@ initable_init (GInitable     *initable,
     pipe_ptrs[2] = &pipe_fds[2];
 #ifdef G_OS_UNIX
   else if (self->flags & G_SUBPROCESS_FLAGS_STDERR_MERGE)
-    /* This will work because stderr gets setup after stdout. */
-    child_data.fds[2] = 1;
+    /* This will work because stderr gets set up after stdout. */
+    stderr_fd = 1;
   else if (self->launcher)
     {
       if (self->launcher->stderr_fd != -1)
-        child_data.fds[2] = self->launcher->stderr_fd;
+        stderr_fd = self->launcher->stderr_fd;
       else if (self->launcher->stderr_path != NULL)
         {
-          child_data.fds[2] = close_fds[2] = unix_open_file (self->launcher->stderr_path, O_CREAT | 
O_WRONLY, error);
-          if (child_data.fds[2] == -1)
+          stderr_fd = close_fds[2] = unix_open_file (self->launcher->stderr_path, O_CREAT | O_WRONLY, error);
+          if (stderr_fd == -1)
             goto out;
         }
     }
 #endif
 
-#ifdef G_OS_UNIX
-  if (self->launcher)
-    {
-      child_data.basic_fd_assignments = self->launcher->basic_fd_assignments;
-      child_data.needdup_fd_assignments = self->launcher->needdup_fd_assignments;
-    }
-#endif
-
   /* argv0 has no '/' in it?  We better do a PATH lookup. */
   if (strchr (self->argv[0], G_DIR_SEPARATOR) == NULL)
     {
@@ -554,23 +392,25 @@ initable_init (GInitable     *initable,
   spawn_flags |= G_SPAWN_DO_NOT_REAP_CHILD;
   spawn_flags |= G_SPAWN_CLOEXEC_PIPES;
 
+  success = g_spawn_async_with_pipes_and_fds (self->launcher ? self->launcher->cwd : NULL,
+                                              (const gchar * const *) self->argv,
+                                              (const gchar * const *) (self->launcher ? self->launcher->envp 
: NULL),
+                                              spawn_flags,
 #ifdef G_OS_UNIX
-  child_data.child_setup_func = self->launcher ? self->launcher->child_setup_func : NULL;
-  child_data.child_setup_data = self->launcher ? self->launcher->child_setup_user_data : NULL;
-#endif
-
-  success = g_spawn_async_with_pipes (self->launcher ? self->launcher->cwd : NULL,
-                                      self->argv,
-                                      self->launcher ? self->launcher->envp : NULL,
-                                      spawn_flags,
-#ifdef G_OS_UNIX
-                                      child_setup, &child_data,
+                                              self->launcher ? self->launcher->child_setup_func : NULL,
+                                              self->launcher ? self->launcher->child_setup_user_data : NULL,
+                                              stdin_fd, stdout_fd, stderr_fd,
+                                              self->launcher ? (const gint *) 
self->launcher->source_fds->data : NULL,
+                                              self->launcher ? (const gint *) 
self->launcher->target_fds->data : NULL,
+                                              self->launcher ? self->launcher->source_fds->len : 0,
 #else
-                                      NULL, NULL,
+                                              NULL, NULL,
+                                              -1, -1, -1,
+                                              NULL, NULL, 0,
 #endif
-                                      &self->pid,
-                                      pipe_ptrs[0], pipe_ptrs[1], pipe_ptrs[2],
-                                      error);
+                                              &self->pid,
+                                              pipe_ptrs[0], pipe_ptrs[1], pipe_ptrs[2],
+                                              error);
   g_assert (success == (self->pid != 0));
 
   {
diff --git a/gio/gsubprocesslauncher-private.h b/gio/gsubprocesslauncher-private.h
index 95431a0ab..f8a6516c5 100644
--- a/gio/gsubprocesslauncher-private.h
+++ b/gio/gsubprocesslauncher-private.h
@@ -42,8 +42,8 @@ struct _GSubprocessLauncher
   gint stderr_fd;
   gchar *stderr_path;
 
-  GArray *basic_fd_assignments;
-  GArray *needdup_fd_assignments;
+  GArray *source_fds;
+  GArray *target_fds;  /* always the same length as source_fds */
   gboolean closed_fd;
 
   GSpawnChildSetupFunc child_setup_func;
diff --git a/gio/gsubprocesslauncher.c b/gio/gsubprocesslauncher.c
index 9e077bd20..b7257f453 100644
--- a/gio/gsubprocesslauncher.c
+++ b/gio/gsubprocesslauncher.c
@@ -164,8 +164,8 @@ g_subprocess_launcher_init (GSubprocessLauncher  *self)
   self->stdin_fd = -1;
   self->stdout_fd = -1;
   self->stderr_fd = -1;
-  self->basic_fd_assignments = g_array_new (FALSE, 0, sizeof (int));
-  self->needdup_fd_assignments = g_array_new (FALSE, 0, sizeof (int));
+  self->source_fds = g_array_new (FALSE, 0, sizeof (int));
+  self->target_fds = g_array_new (FALSE, 0, sizeof (int));
 #endif
 }
 
@@ -613,18 +613,10 @@ g_subprocess_launcher_take_fd (GSubprocessLauncher   *self,
                                gint                   source_fd,
                                gint                   target_fd)
 {
-  if (source_fd == target_fd)
+  if (self->source_fds != NULL && self->target_fds != NULL)
     {
-      if (self->basic_fd_assignments)
-        g_array_append_val (self->basic_fd_assignments, source_fd);
-    }
-  else
-    {
-      if (self->needdup_fd_assignments)
-        {
-          g_array_append_val (self->needdup_fd_assignments, source_fd);
-          g_array_append_val (self->needdup_fd_assignments, target_fd);
-        }
+      g_array_append_val (self->source_fds, source_fd);
+      g_array_append_val (self->target_fds, target_fd);
     }
 }
 
@@ -664,17 +656,18 @@ g_subprocess_launcher_close (GSubprocessLauncher *self)
     close (self->stderr_fd);
   self->stderr_fd = -1;
 
-  if (self->basic_fd_assignments)
+  if (self->source_fds)
     {
-      for (i = 0; i < self->basic_fd_assignments->len; i++)
-        (void) close (g_array_index (self->basic_fd_assignments, int, i));
-      g_clear_pointer (&self->basic_fd_assignments, g_array_unref);
-    }
-  if (self->needdup_fd_assignments)
-    {
-      for (i = 0; i < self->needdup_fd_assignments->len; i += 2)
-        (void) close (g_array_index (self->needdup_fd_assignments, int, i));
-      g_clear_pointer (&self->needdup_fd_assignments, g_array_unref);
+      g_assert (self->target_fds != NULL);
+      g_assert (self->source_fds->len == self->target_fds->len);
+
+      for (i = 0; i < self->source_fds->len; i++)
+        {
+          (void) close (g_array_index (self->source_fds, int, i));
+          (void) close (g_array_index (self->target_fds, int, i));
+        }
+      g_clear_pointer (&self->source_fds, g_array_unref);
+      g_clear_pointer (&self->target_fds, g_array_unref);
     }
 
   self->closed_fd = TRUE;