Merge branch '2332-subprocess-launcher-oops' into 'master'

gsubprocesslauncher: Don’t close target FDs in close() method

Closes #2332

See merge request GNOME/glib!1958

3 files changed, 34 insertions, 13 deletions

diff --git a/gio/gsubprocesslauncher-private.h b/gio/gsubprocesslauncher-private.h
index f8a6516c5..d6fe0d784 100644
--- a/gio/gsubprocesslauncher-private.h
+++ b/gio/gsubprocesslauncher-private.h
@@ -42,8 +42,8 @@ struct _GSubprocessLauncher
   gint stderr_fd;
   gchar *stderr_path;
 
-  GArray *source_fds;
-  GArray *target_fds;  /* always the same length as source_fds */
+  GArray *source_fds;  /* GSubprocessLauncher has ownership of the FD elements */
+  GArray *target_fds;  /* always the same length as source_fds; elements are just integers and not FDs in this process */
   gboolean closed_fd;
 
   GSpawnChildSetupFunc child_setup_func;
diff --git a/gio/gsubprocesslauncher.c b/gio/gsubprocesslauncher.c
index b7257f453..a1c65e947 100644
--- a/gio/gsubprocesslauncher.c
+++ b/gio/gsubprocesslauncher.c
@@ -596,16 +596,16 @@ g_subprocess_launcher_take_stderr_fd (GSubprocessLauncher *self,
  * @target_fd: Target descriptor for child process
  *
  * Transfer an arbitrary file descriptor from parent process to the
- * child.  This function takes "ownership" of the fd; it will be closed
+ * child.  This function takes ownership of the @source_fd; it will be closed
  * in the parent when @self is freed.
  *
  * By default, all file descriptors from the parent will be closed.
- * This function allows you to create (for example) a custom pipe() or
- * socketpair() before launching the process, and choose the target
+ * This function allows you to create (for example) a custom `pipe()` or
+ * `socketpair()` before launching the process, and choose the target
  * descriptor in the child.
  *
  * An example use case is GNUPG, which has a command line argument
- * --passphrase-fd providing a file descriptor number where it expects
+ * `--passphrase-fd` providing a file descriptor number where it expects
  * the passphrase to be written.
  */
 void
@@ -661,11 +661,11 @@ g_subprocess_launcher_close (GSubprocessLauncher *self)
       g_assert (self->target_fds != NULL);
       g_assert (self->source_fds->len == self->target_fds->len);
 
+      /* Note: Don’t close the target_fds, as they’re only valid FDs in the
+       * child process. This code never executes in the child process. */
       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));
-        }
+        (void) close (g_array_index (self->source_fds, int, i));
+
       g_clear_pointer (&self->source_fds, g_array_unref);
       g_clear_pointer (&self->target_fds, g_array_unref);
     }
diff --git a/gio/tests/gsubprocess.c b/gio/tests/gsubprocess.c
index 3c248e610..7e22678ec 100644
--- a/gio/tests/gsubprocess.c
+++ b/gio/tests/gsubprocess.c
@@ -1494,23 +1494,44 @@ test_subprocess_launcher_close (void)
   GSubprocessLauncher *launcher;
   GSubprocess *proc;
   GPtrArray *args;
-  int fd;
+  int fd, fd2;
   gboolean is_open;
 
-  fd = dup(0);
+  /* Open two arbitrary FDs. One of them, @fd, will be transferred to the
+   * launcher, and the other’s FD integer will be used as its target FD, giving
+   * the mapping `fd → fd2` if a child process were to be spawned.
+   *
+   * The launcher will then be closed, which should close @fd but *not* @fd2,
+   * as the value of @fd2 is only valid as an FD in a child process. (A child
+   * process is not actually spawned in this test.)
+   */
+  fd = dup (0);
+  fd2 = dup (0);
   launcher = g_subprocess_launcher_new (G_SUBPROCESS_FLAGS_NONE);
-  g_subprocess_launcher_take_fd (launcher, fd, fd);
+  g_subprocess_launcher_take_fd (launcher, fd, fd2);
+
   is_open = fcntl (fd, F_GETFD) != -1;
   g_assert_true (is_open);
+  is_open = fcntl (fd2, F_GETFD) != -1;
+  g_assert_true (is_open);
+
   g_subprocess_launcher_close (launcher);
+
   is_open = fcntl (fd, F_GETFD) != -1;
   g_assert_false (is_open);
+  is_open = fcntl (fd2, F_GETFD) != -1;
+  g_assert_true (is_open);
+
+  /* Now test that actually trying to spawn the child gives %G_IO_ERROR_CLOSED,
+   * as g_subprocess_launcher_close() has been called. */
   args = get_test_subprocess_args ("cat", NULL);
   proc = g_subprocess_launcher_spawnv (launcher, (const gchar * const *) args->pdata, error);
   g_ptr_array_free (args, TRUE);
   g_assert_null (proc);
   g_assert_error (local_error, G_IO_ERROR, G_IO_ERROR_CLOSED);
   g_clear_error (error);
+
+  close (fd2);
   g_object_unref (launcher);
 }