Merge branch 'mcatanzaro/posix-spawn2' into 'main'

gspawn: Implement fd remapping for posix_spawn codepath, and fix file descriptor conflation issues

Closes #2503 and #2506

See merge request GNOME/glib!1968

gio/tests/gsubprocess-testprog.c

@@ -5,8 +5,6 @@
 #include <errno.h>
 #ifdef G_OS_UNIX
 #include <unistd.h>
-#include <gio/gunixinputstream.h>
-#include <gio/gunixoutputstream.h>
 #else
 #include <io.h>
 #endif
@@ -150,6 +148,55 @@ write_to_fds (int argc, char **argv)
   return 0;
 }
 
+static int
+read_from_fd (int argc, char **argv)
+{
+  int fd;
+  const char expected_result[] = "Yay success!";
+  guint8 buf[sizeof (expected_result) + 1];
+  gsize bytes_read;
+  FILE *f;
+
+  if (argc != 3)
+    {
+      g_print ("Usage: %s read-from-fd FD\n", argv[0]);
+      return 1;
+    }
+
+  fd = atoi (argv[2]);
+  if (fd == 0)
+    {
+      g_warning ("Argument \"%s\" does not look like a valid nonzero file descriptor", argv[2]);
+      return 1;
+    }
+
+  f = fdopen (fd, "r");
+  if (f == NULL)
+    {
+      g_warning ("Failed to open fd %d: %s", fd, g_strerror (errno));
+      return 1;
+    }
+
+  bytes_read = fread (buf, 1, sizeof (buf), f);
+  if (bytes_read != sizeof (expected_result))
+    {
+      g_warning ("Read %zu bytes, but expected %zu", bytes_read, sizeof (expected_result));
+      return 1;
+    }
+
+  if (memcmp (expected_result, buf, sizeof (expected_result)) != 0)
+    {
+      buf[sizeof (expected_result)] = '\0';
+      g_warning ("Expected \"%s\" but read  \"%s\"", expected_result, (char *)buf);
+      return 1;
+    }
+
+  if (fclose (f) == -1)
+    g_assert_not_reached ();
+
+  return 0;
+}
+
 static int
 env_mode (int argc, char **argv)
 {
@@ -242,6 +289,8 @@ main (int argc, char **argv)
     return sleep_forever_mode (argc, argv);
   else if (strcmp (mode, "write-to-fds") == 0)
     return write_to_fds (argc, argv);
+  else if (strcmp (mode, "read-from-fd") == 0)
+    return read_from_fd (argc, argv);
   else if (strcmp (mode, "env") == 0)
     return env_mode (argc, argv);
   else if (strcmp (mode, "cwd") == 0)

gio/tests/gsubprocess.c

@@ -5,6 +5,7 @@
 #include <sys/wait.h>
 #include <glib-unix.h>
 #include <gio/gunixinputstream.h>
+#include <gio/gunixoutputstream.h>
 #include <gio/gfiledescriptorbased.h>
 #include <unistd.h>
 #include <fcntl.h>
@@ -1723,7 +1724,8 @@ test_child_setup (void)
 }
 
 static void
-test_pass_fd (void)
+do_test_pass_fd (GSubprocessFlags     flags,
+                 GSpawnChildSetupFunc child_setup)
 {
   GError *local_error = NULL;
   GError **error = &local_error;
@@ -1748,9 +1750,11 @@ test_pass_fd (void)
   needdup_fd_str = g_strdup_printf ("%d", needdup_pipefds[1] + 1);
 
   args = get_test_subprocess_args ("write-to-fds", basic_fd_str, needdup_fd_str, NULL);
-  launcher = g_subprocess_launcher_new (G_SUBPROCESS_FLAGS_NONE);
+  launcher = g_subprocess_launcher_new (flags);
   g_subprocess_launcher_take_fd (launcher, basic_pipefds[1], basic_pipefds[1]);
   g_subprocess_launcher_take_fd (launcher, needdup_pipefds[1], needdup_pipefds[1] + 1);
+  if (child_setup != NULL)
+    g_subprocess_launcher_set_child_setup (launcher, child_setup, NULL, NULL);
   proc = g_subprocess_launcher_spawnv (launcher, (const gchar * const *) args->pdata, error);
   g_ptr_array_free (args, TRUE);
   g_assert_no_error (local_error);
@@ -1780,6 +1784,206 @@ test_pass_fd (void)
   g_object_unref (proc);
 }
 
+static void
+test_pass_fd (void)
+{
+  do_test_pass_fd (G_SUBPROCESS_FLAGS_NONE, NULL);
+}
+
+static void
+empty_child_setup (gpointer user_data)
+{
+}
+
+static void
+test_pass_fd_empty_child_setup (void)
+{
+  /* Using a child setup function forces gspawn to use fork/exec
+   * rather than posix_spawn.
+   */
+  do_test_pass_fd (G_SUBPROCESS_FLAGS_NONE, empty_child_setup);
+}
+
+static void
+test_pass_fd_inherit_fds (void)
+{
+  /* Try to test the optimized posix_spawn codepath instead of
+   * fork/exec. Currently this requires using INHERIT_FDS since gspawn's
+   * posix_spawn codepath does not currently handle closing
+   * non-inherited fds. Note that using INHERIT_FDS means our testing of
+   * g_subprocess_launcher_take_fd() is less-comprehensive than when
+   * using G_SUBPROCESS_FLAGS_NONE.
+   */
+  do_test_pass_fd (G_SUBPROCESS_FLAGS_INHERIT_FDS, NULL);
+}
+
+static void
+do_test_fd_conflation (GSubprocessFlags     flags,
+                       GSpawnChildSetupFunc child_setup,
+                       gboolean             test_child_err_report_fd)
+{
+  char success_message[] = "Yay success!";
+  GError *error = NULL;
+  GOutputStream *output_stream;
+  GSubprocessLauncher *launcher;
+  GSubprocess *proc;
+  GPtrArray *args;
+  int unused_pipefds[2];
+  int pipefds[2];
+  int fd_to_pass_to_child;
+  gsize bytes_written;
+  gboolean success;
+  char *fd_str;
+
+  /* This test must run in a new process because it is extremely sensitive to
+   * order of opened fds.
+   */
+  if (!g_test_subprocess ())
+    {
+      g_test_trap_subprocess (NULL, 0, G_TEST_SUBPROCESS_INHERIT_STDOUT | G_TEST_SUBPROCESS_INHERIT_STDERR);
+      g_test_trap_assert_passed ();
+      return;
+    }
+
+  g_unix_open_pipe (unused_pipefds, FD_CLOEXEC, &error);
+  g_assert_no_error (error);
+
+  g_unix_open_pipe (pipefds, FD_CLOEXEC, &error);
+  g_assert_no_error (error);
+
+  /* The fds should be sequential since we are in a new process. */
+  g_assert_cmpint (unused_pipefds[0] /* 3 */, ==, unused_pipefds[1] - 1);
+  g_assert_cmpint (unused_pipefds[1] /* 4 */, ==, pipefds[0] - 1);
+  g_assert_cmpint (pipefds[0] /* 5 */, ==, pipefds[1] /* 6 */ - 1);
+
+  /* Because GSubprocess allows arbitrary remapping of fds, it has to be careful
+   * to avoid fd conflation issues, e.g. it should properly handle 5 -> 4 and
+   * 4 -> 5 at the same time. GIO previously attempted to handle this by naively
+   * dup'ing the source fds, but this was not good enough because it was
+   * possible that the dup'ed result could still conflict with one of the target
+   * fds. For example:
+   *
+   * source_fd 5 -> target_fd 9, source_fd 3 -> target_fd 7
+   *
+   * dup(5) -> dup returns 8
+   * dup(3) -> dup returns 9
+   *
+   * After dup'ing, we wind up with: 8 -> 9, 9 -> 7. That means that after we
+   * dup2(8, 9), we have clobbered fd 9 before we dup2(9, 7). The end result is
+   * we have remapped 5 -> 9 as expected, but then remapped 5 -> 7 instead of
+   * 3 -> 7 as the application intended.
+   *
+   * This issue has been fixed in the simplest way possible, by passing a
+   * minimum fd value when using F_DUPFD_CLOEXEC that is higher than any of the
+   * target fds, to guarantee all source fds are different than all target fds,
+   * eliminating any possibility of conflation.
+   *
+   * Anyway, that is why we have the unused_pipefds here. We need to open fds in
+   * a certain order in order to trick older GSubprocess into conflating the
+   * fds. The primary goal of this test is to ensure this particular conflation
+   * issue is not reintroduced. See glib#2503.
+   *
+   * This test also has an alternate mode of operation where it instead tests
+   * for conflation with gspawn's child_err_report_fd, glib#2506.
+   *
+   * Be aware this test is necessarily extremely fragile. To reproduce these
+   * bugs, it relies on internals of gspawn and gmain that will likely change
+   * in the future, eventually causing this test to no longer test the bugs
+   * it was originally designed to test. That is OK! If the test fails, at
+   * least you know *something* is wrong.
+   */
+  if (test_child_err_report_fd)
+    fd_to_pass_to_child = pipefds[1] + 2 /* 8 */;
+  else
+    fd_to_pass_to_child = pipefds[1] + 3 /* 9 */;
+
+  launcher = g_subprocess_launcher_new (flags);
+  g_subprocess_launcher_take_fd (launcher, pipefds[0] /* 5 */, fd_to_pass_to_child);
+  g_subprocess_launcher_take_fd (launcher, unused_pipefds[0] /* 3 */, pipefds[1] + 1 /* 7 */);
+  if (child_setup != NULL)
+    g_subprocess_launcher_set_child_setup (launcher, child_setup, NULL, NULL);
+  fd_str = g_strdup_printf ("%d", fd_to_pass_to_child);
+  args = get_test_subprocess_args ("read-from-fd", fd_str, NULL);
+  proc = g_subprocess_launcher_spawnv (launcher, (const gchar * const *) args->pdata, &error);
+  g_assert_no_error (error);
+  g_assert_nonnull (proc);
+  g_ptr_array_free (args, TRUE);
+  g_object_unref (launcher);
+  g_free (fd_str);
+
+  /* Close the read ends of the pipes. */
+  close (unused_pipefds[0]);
+  close (pipefds[0]);
+
+  /* Also close the write end of the unused pipe. */
+  close (unused_pipefds[1]);
+
+  /* If doing our normal test:
+   *
+   * So now pipefds[0] should be inherited into the subprocess as
+   * pipefds[1] + 2, and unused_pipefds[0] should be inherited as
+   * pipefds[1] + 1. We will write to pipefds[1] and the subprocess will verify
+   * that it reads the expected data. But older broken GIO will accidentally
+   * clobber pipefds[1] + 2 with pipefds[1] + 1! This will cause the subprocess
+   * to hang trying to read from the wrong pipe.
+   *
+   * If testing conflation with child_err_report_fd:
+   *
+   * We are actually already done. The real test succeeded if we made it this
+   * far without hanging while spawning the child. But let's continue with our
+   * write and read anyway, to ensure things are good.
+   */
+  output_stream = g_unix_output_stream_new (pipefds[1], TRUE);
+  success = g_output_stream_write_all (output_stream,
+                                       success_message, sizeof (success_message),
+                                       &bytes_written,
+                                       NULL,
+                                       &error);
+  g_assert_no_error (error);
+  g_assert_cmpint (bytes_written, ==, sizeof (success_message));
+  g_assert_true (success);
+  g_object_unref (output_stream);
+
+  success = g_subprocess_wait_check (proc, NULL, &error);
+  g_assert_no_error (error);
+  g_object_unref (proc);
+}
+
+static void
+test_fd_conflation (void)
+{
+  do_test_fd_conflation (G_SUBPROCESS_FLAGS_NONE, NULL, FALSE);
+}
+
+static void
+test_fd_conflation_empty_child_setup (void)
+{
+  /* Using a child setup function forces gspawn to use fork/exec
+   * rather than posix_spawn.
+   */
+  do_test_fd_conflation (G_SUBPROCESS_FLAGS_NONE, empty_child_setup, FALSE);
+}
+
+static void
+test_fd_conflation_inherit_fds (void)
+{
+  /* Try to test the optimized posix_spawn codepath instead of
+   * fork/exec. Currently this requires using INHERIT_FDS since gspawn's
+   * posix_spawn codepath does not currently handle closing
+   * non-inherited fds.
+   */
+  do_test_fd_conflation (G_SUBPROCESS_FLAGS_INHERIT_FDS, NULL, FALSE);
+}
+
+static void
+test_fd_conflation_child_err_report_fd (void)
+{
+  /* Using a child setup function forces gspawn to use fork/exec
+   * rather than posix_spawn.
+   */
+  do_test_fd_conflation (G_SUBPROCESS_FLAGS_NONE, empty_child_setup, TRUE);
+}
+
 #endif
 
 static void
@@ -1917,7 +2121,13 @@ main (int argc, char **argv)
   g_test_add_func ("/gsubprocess/stdout-file", test_stdout_file);
   g_test_add_func ("/gsubprocess/stdout-fd", test_stdout_fd);
   g_test_add_func ("/gsubprocess/child-setup", test_child_setup);
-  g_test_add_func ("/gsubprocess/pass-fd", test_pass_fd);
+  g_test_add_func ("/gsubprocess/pass-fd/basic", test_pass_fd);
+  g_test_add_func ("/gsubprocess/pass-fd/empty-child-setup", test_pass_fd_empty_child_setup);
+  g_test_add_func ("/gsubprocess/pass-fd/inherit-fds", test_pass_fd_inherit_fds);
+  g_test_add_func ("/gsubprocess/fd-conflation/basic", test_fd_conflation);
+  g_test_add_func ("/gsubprocess/fd-conflation/empty-child-setup", test_fd_conflation_empty_child_setup);
+  g_test_add_func ("/gsubprocess/fd-conflation/inherit-fds", test_fd_conflation_inherit_fds);
+  g_test_add_func ("/gsubprocess/fd-conflation/child-err-report-fd", test_fd_conflation_child_err_report_fd);
 #endif
   g_test_add_func ("/gsubprocess/launcher-environment", test_launcher_environment);
 

glib/gspawn.c

@@ -1298,13 +1298,13 @@ unset_cloexec (int fd)
 /* This function is called between fork() and exec() and hence must be
  * async-signal-safe (see signal-safety(7)). */
 static int
-dupfd_cloexec (int parent_fd)
+dupfd_cloexec (int old_fd, int new_fd_min)
 {
   int fd, errsv;
 #ifdef F_DUPFD_CLOEXEC
   do
     {
-      fd = fcntl (parent_fd, F_DUPFD_CLOEXEC, 3);
+      fd = fcntl (old_fd, F_DUPFD_CLOEXEC, new_fd_min);
       errsv = errno;
     }
   while (fd == -1 && errsv == EINTR);
@@ -1315,7 +1315,7 @@ dupfd_cloexec (int parent_fd)
   int result, flags;
   do
     {
-      fd = fcntl (parent_fd, F_DUPFD, 3);
+      fd = fcntl (old_fd, F_DUPFD, new_fd_min);
       errsv = errno;
     }
   while (fd == -1 && errsv == EINTR);
@@ -1588,20 +1588,6 @@ safe_closefrom (int lowfd)
 #endif
 }
 
-/* This function is called between fork() and exec() and hence must be
- * async-signal-safe (see signal-safety(7)). */
-static gint
-safe_dup (gint fd)
-{
-  gint ret;
-
-  do
-    ret = dup (fd);
-  while (ret < 0 && (errno == EINTR || errno == EBUSY));
-
-  return ret;
-}
-
 /* This function is called between fork() and exec() and hence must be
  * async-signal-safe (see signal-safety(7)). */
 static gint
@@ -1634,6 +1620,7 @@ enum
 {
   CHILD_CHDIR_FAILED,
   CHILD_EXEC_FAILED,
+  CHILD_OPEN_FAILED,
   CHILD_DUP2_FAILED,
   CHILD_FORK_FAILED
 };
@@ -1665,6 +1652,7 @@ do_exec (gint                  child_err_report_fd,
          gpointer              user_data)
 {
   gsize i;
+  gint max_target_fd = 0;
 
   if (working_directory && chdir (working_directory) < 0)
     write_err_and_exit (child_err_report_fd,
@@ -1673,7 +1661,6 @@ do_exec (gint                  child_err_report_fd,
   /* Redirect pipes as required */
   if (stdin_fd >= 0)
     {
-      /* dup2 can't actually fail here I don't think */
       if (safe_dup2 (stdin_fd, 0) < 0)
         write_err_and_exit (child_err_report_fd,
                             CHILD_DUP2_FAILED);
@@ -1687,15 +1674,16 @@ do_exec (gint                  child_err_report_fd,
       /* Keep process from blocking on a read of stdin */
       gint read_null = safe_open ("/dev/null", O_RDONLY);
       if (read_null < 0)
+        write_err_and_exit (child_err_report_fd,
+                            CHILD_OPEN_FAILED);
+      if (safe_dup2 (read_null, 0) < 0)
         write_err_and_exit (child_err_report_fd,
                             CHILD_DUP2_FAILED);
-      safe_dup2 (read_null, 0);
       close_and_invalidate (&read_null);
     }
 
   if (stdout_fd >= 0)
     {
-      /* dup2 can't actually fail here I don't think */
       if (safe_dup2 (stdout_fd, 1) < 0)
         write_err_and_exit (child_err_report_fd,
                             CHILD_DUP2_FAILED);
@@ -1708,15 +1696,16 @@ do_exec (gint                  child_err_report_fd,
     {
       gint write_null = safe_open ("/dev/null", O_WRONLY);
       if (write_null < 0)
+        write_err_and_exit (child_err_report_fd,
+                            CHILD_OPEN_FAILED);
+      if (safe_dup2 (write_null, 1) < 0)
         write_err_and_exit (child_err_report_fd,
                             CHILD_DUP2_FAILED);
-      safe_dup2 (write_null, 1);
       close_and_invalidate (&write_null);
     }
 
   if (stderr_fd >= 0)
     {
-      /* dup2 can't actually fail here I don't think */
       if (safe_dup2 (stderr_fd, 2) < 0)
         write_err_and_exit (child_err_report_fd,
                             CHILD_DUP2_FAILED);
@@ -1729,9 +1718,11 @@ do_exec (gint                  child_err_report_fd,
     {
       gint write_null = safe_open ("/dev/null", O_WRONLY);
       if (write_null < 0)
+        write_err_and_exit (child_err_report_fd,
+                            CHILD_OPEN_FAILED);
+      if (safe_dup2 (write_null, 2) < 0)
         write_err_and_exit (child_err_report_fd,
                             CHILD_DUP2_FAILED);
-      safe_dup2 (write_null, 2);
       close_and_invalidate (&write_null);
     }
 
@@ -1744,7 +1735,8 @@ do_exec (gint                  child_err_report_fd,
     {
       if (child_setup == NULL && n_fds == 0)
         {
-          safe_dup2 (child_err_report_fd, 3);
+          if (safe_dup2 (child_err_report_fd, 3) < 0)
+            write_err_and_exit (child_err_report_fd, CHILD_DUP2_FAILED);
           set_cloexec (GINT_TO_POINTER (0), 3);
           safe_closefrom (4);
           child_err_report_fd = 3;
@@ -1763,42 +1755,64 @@ do_exec (gint                  child_err_report_fd,
   /*
    * Work through the @source_fds and @target_fds mapping.
    *
-   * Based on code derived from
+   * Based on code originally derived from
    * gnome-terminal:src/terminal-screen.c:terminal_screen_child_setup(),
-   * used under the LGPLv2+ with permission from author.
+   * used under the LGPLv2+ with permission from author. (The code has
+   * since migrated to vte:src/spawn.cc:SpawnContext::exec and is no longer
+   * terribly similar to what we have here.)
    */
 
-  /* Basic fd assignments (where source == target) we can just unset FD_CLOEXEC
-   *
-   * 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 in a first pass.
-   *
-   * If any of the @target_fds conflict with @child_err_report_fd, dup the
-   * latter so it doesn’t get conflated.
-   */
   if (n_fds > 0)
     {
       for (i = 0; i < n_fds; i++)
+        max_target_fd = MAX (max_target_fd, target_fds[i]);
+
+      if (max_target_fd == G_MAXINT)
         {
-          if (source_fds[i] != target_fds[i])
+          errno = EINVAL;
-            source_fds[i] = dupfd_cloexec (source_fds[i]);
+          write_err_and_exit (child_err_report_fd, CHILD_DUP2_FAILED);
         }
+
+      /* 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 all source fds, taking care to ensure the new
+       * fds are larger than any target fd to avoid introducing new conflicts.
+       */
       for (i = 0; i < n_fds; i++)
         {
+          if (source_fds[i] != target_fds[i])
+            {
+              source_fds[i] = dupfd_cloexec (source_fds[i], max_target_fd + 1);
+              if (source_fds[i] < 0)
+                write_err_and_exit (child_err_report_fd, CHILD_DUP2_FAILED);
+            }
+        }
+
+      for (i = 0; i < n_fds; i++)
+        {
+          /* For basic fd assignments (where source == target), we can just
+           * unset FD_CLOEXEC.
+           */
           if (source_fds[i] == target_fds[i])
             {
               unset_cloexec (source_fds[i]);
             }
           else
             {
+              /* If any of the @target_fds conflict with @child_err_report_fd,
+               * dup it so it doesn’t get conflated.
+               */
               if (target_fds[i] == child_err_report_fd)
-                child_err_report_fd = safe_dup (child_err_report_fd);
+                {
+                  child_err_report_fd = dupfd_cloexec (child_err_report_fd, max_target_fd + 1);
+                  if (child_err_report_fd < 0)
+                    write_err_and_exit (child_err_report_fd, CHILD_DUP2_FAILED);
+                }
 
-              safe_dup2 (source_fds[i], target_fds[i]);
+              if (safe_dup2 (source_fds[i], target_fds[i]) < 0)
-              (void) close (source_fds[i]);
+                write_err_and_exit (child_err_report_fd, CHILD_DUP2_FAILED);
+
+              close_and_invalidate (&source_fds[i]);
             }
         }
     }
@@ -1881,18 +1895,24 @@ do_posix_spawn (const gchar * const *argv,
                 gint       *child_close_fds,
                 gint        stdin_fd,
                 gint        stdout_fd,
-                gint        stderr_fd)
+                gint        stderr_fd,
+                const gint *source_fds,
+                const gint *target_fds,
+                gsize       n_fds)
 {
   pid_t pid;
+  gint *duped_source_fds = NULL;
+  gint max_target_fd = 0;
   const gchar * const *argv_pass;
   posix_spawnattr_t attr;
   posix_spawn_file_actions_t file_actions;
   gint parent_close_fds[3];
-  gint num_parent_close_fds = 0;
+  gsize num_parent_close_fds = 0;
   GSList *child_close = NULL;
   GSList *elem;
   sigset_t mask;
-  int i, r;
+  gsize i;
+  int r;
 
   if (*argv[0] == '\0')
     {
@@ -2006,6 +2026,50 @@ do_posix_spawn (const gchar * const *argv,
         goto out_close_fds;
     }
 
+  /* If source_fds[i] != target_fds[i], 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, taking care to ensure the new fds are
+   * larger than any target fd to avoid introducing new conflicts.
+   *
+   * If source_fds[i] == target_fds[i], then we just need to leak
+   * the fd into the child process, which we *could* do by temporarily
+   * unsetting CLOEXEC and then setting it again after we spawn if
+   * it was originally set. POSIX requires that the addup2 action unset
+   * CLOEXEC if source and target are identical, so you'd think doing it
+   * manually wouldn't be needed, but unfortunately as of 2021 many
+   * libcs still don't do so. Example nonconforming libcs:
+   *  Bionic: https://android.googlesource.com/platform/bionic/+/f6e5b582604715729b09db3e36a7aeb8c24b36a4/libc/bionic/spawn.cpp#71
+   *  uclibc-ng: https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/librt/spawn.c?id=7c36bcae09d66bbaa35cbb02253ae0556f42677e#n88
+   *
+   * Anyway, unsetting CLOEXEC ourselves would open a small race window
+   * where the fd could be inherited into a child process if another
+   * thread spawns something at the same time, because we have not
+   * called fork() and are multithreaded here. This race is avoidable by
+   * using dupfd_cloexec, which we already have to do to handle the
+   * source_fds[i] != target_fds[i] case. So let's always do it!
+   */
+
+  for (i = 0; i < n_fds; i++)
+    max_target_fd = MAX (max_target_fd, target_fds[i]);
+
+  if (max_target_fd == G_MAXINT)
+    goto out_close_fds;
+
+  duped_source_fds = g_new (gint, n_fds);
+  for (i = 0; i < n_fds; i++)
+    {
+      duped_source_fds[i] = dupfd_cloexec (source_fds[i], max_target_fd + 1);
+      if (duped_source_fds[i] < 0)
+        goto out_close_fds;
+    }
+
+  for (i = 0; i < n_fds; i++)
+    {
+      r = posix_spawn_file_actions_adddup2 (&file_actions, duped_source_fds[i], target_fds[i]);
+      if (r != 0)
+        goto out_close_fds;
+    }
+
   /* Intentionally close the fds in the child as the last file action,
    * having been careful not to add the same fd to this list twice.
    *
@@ -2038,6 +2102,13 @@ out_close_fds:
   for (i = 0; i < num_parent_close_fds; i++)
     close_and_invalidate (&parent_close_fds [i]);
 
+  if (duped_source_fds != NULL)
+    {
+      for (i = 0; i < n_fds; i++)
+        close_and_invalidate (&duped_source_fds[i]);
+      g_free (duped_source_fds);
+    }
+
   posix_spawn_file_actions_destroy (&file_actions);
 out_free_spawnattr:
   posix_spawnattr_destroy (&attr);
@@ -2125,10 +2196,8 @@ fork_exec (gboolean              intermediate_child,
   child_close_fds[n_child_close_fds++] = -1;
 
 #ifdef POSIX_SPAWN_AVAILABLE
-  /* FIXME: Handle @source_fds and @target_fds in do_posix_spawn() using the
-   * file actions API. */
   if (!intermediate_child && working_directory == NULL && !close_descriptors &&
-      !search_path_from_envp && child_setup == NULL && n_fds == 0)
+      !search_path_from_envp && child_setup == NULL)
     {
       g_trace_mark (G_TRACE_CURRENT_TIME, 0,
                     "GLib", "posix_spawn",
@@ -2145,7 +2214,10 @@ fork_exec (gboolean              intermediate_child,
                                child_close_fds,
                                stdin_fd,
                                stdout_fd,
-                               stderr_fd);
+                               stderr_fd,
+                               source_fds,
+                               target_fds,
+                               n_fds);
       if (status == 0)
         goto success;
 
@@ -2447,11 +2519,19 @@ fork_exec (gboolean              intermediate_child,
 
               break;
 
+            case CHILD_OPEN_FAILED:
+              g_set_error (error,
+                           G_SPAWN_ERROR,
+                           G_SPAWN_ERROR_FAILED,
+                           _("Failed to open file to remap file descriptor (%s)"),
+                           g_strerror (buf[1]));
+              break;
+
             case CHILD_DUP2_FAILED:
               g_set_error (error,
                            G_SPAWN_ERROR,
                            G_SPAWN_ERROR_FAILED,
-                           _("Failed to redirect output or input of child process (%s)"),
+                           _("Failed to duplicate file descriptor for child process (%s)"),
                            g_strerror (buf[1]));
 
               break;