rpms/glib2
3
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity
df270abbbe
glib2/gspawn.patch
Michael Catanzaro 9849a2ff0e Add gspawn patchset
2021-12-06 09:58:23 -06:00

1053 lines
38 KiB
Diff
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

From 62ef7cffb4c71cea3b52defa9e51206f54b718fe Mon Sep 17 00:00:00 2001
From: Michael Catanzaro <mcatanzaro@redhat.com>
Date: Tue, 12 Oct 2021 15:52:18 -0500
Subject: [PATCH 01/10] gspawn: use close_and_invalidate more
---
glib/gspawn.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/glib/gspawn.c b/glib/gspawn.c
index 51b4c79ac..2b191634c 100644
--- a/glib/gspawn.c
+++ b/glib/gspawn.c
@@ -1798,7 +1798,7 @@ do_exec (gint child_err_report_fd,
child_err_report_fd = safe_dup (child_err_report_fd);
safe_dup2 (source_fds[i], target_fds[i]);
- (void) close (source_fds[i]);
+ close_and_invalidate (&source_fds[i]);
}
}
}
--
2.33.1
From adddd12707bc2bfd14ad35ae8075eeac68cdd95c Mon Sep 17 00:00:00 2001
From: Michael Catanzaro <mcatanzaro@redhat.com>
Date: Thu, 14 Oct 2021 10:43:52 -0500
Subject: [PATCH 02/10] gspawn: Improve error message when dup fails
This error message is no longer accurate now that we allow arbitrary fd
remapping.
---
glib/gspawn.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/glib/gspawn.c b/glib/gspawn.c
index 2b191634c..9c65d9d92 100644
--- a/glib/gspawn.c
+++ b/glib/gspawn.c
@@ -2451,7 +2451,7 @@ fork_exec (gboolean intermediate_child,
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;
--
2.33.1
From 976558ec8b4125323e4c7adb600c8249f5319d5f Mon Sep 17 00:00:00 2001
From: Michael Catanzaro <mcatanzaro@redhat.com>
Date: Tue, 12 Oct 2021 15:33:59 -0500
Subject: [PATCH 03/10] gspawn: fix hangs when duping child_err_report_fd
In case child_err_report_fd conflicts with one of the target_fds, the
code here is careful to dup child_err_report_fd in order to avoid
conflating the two. It was a good idea, but evidently was not tested,
because the newly-created fd is not created with CLOEXEC set. This means
it stays open in the child process, causing the parent to hang forever
waiting to read from the other end of the pipe. Oops!
The fix is simple: just set CLOEXEC. This removes our only usage of the
safe_dup() function, so it can be dropped.
Fixes #2506
---
glib/gspawn.c | 16 +---------------
1 file changed, 1 insertion(+), 15 deletions(-)
diff --git a/glib/gspawn.c b/glib/gspawn.c
index 9c65d9d92..0deeda8cc 100644
--- a/glib/gspawn.c
+++ b/glib/gspawn.c
@@ -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
@@ -1795,7 +1781,7 @@ do_exec (gint child_err_report_fd,
else
{
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);
safe_dup2 (source_fds[i], target_fds[i]);
close_and_invalidate (&source_fds[i]);
--
2.33.1
From 702b7c56dd314392f10499f9d62ed3fd4bf2f498 Mon Sep 17 00:00:00 2001
From: Michael Catanzaro <mcatanzaro@redhat.com>
Date: Thu, 14 Oct 2021 10:44:57 -0500
Subject: [PATCH 04/10] gspawn: fix fd remapping conflation issue
We currently dup all source fds to avoid possible conflation with the
target fds, but fail to consider that the result of a dup might itself
conflict with one of the target fds. Solve this the easy way by duping
all source_fds to values that are greater than the largest fd in
target_fds.
Fixes #2503
---
glib/gspawn.c | 43 +++++++++++++++++++++++++------------------
1 file changed, 25 insertions(+), 18 deletions(-)
diff --git a/glib/gspawn.c b/glib/gspawn.c
index 0deeda8cc..149f5a5a1 100644
--- a/glib/gspawn.c
+++ b/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);
@@ -1651,6 +1651,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,
@@ -1749,39 +1750,45 @@ 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 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]);
+ source_fds[i] = dupfd_cloexec (source_fds[i], max_target_fd + 1);
}
+
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 = dupfd_cloexec (child_err_report_fd);
+ child_err_report_fd = dupfd_cloexec (child_err_report_fd, max_target_fd + 1);
safe_dup2 (source_fds[i], target_fds[i]);
close_and_invalidate (&source_fds[i]);
--
2.33.1
From 6324ec1cd33b923f543099bd977636772ba8ae3e Mon Sep 17 00:00:00 2001
From: Michael Catanzaro <mcatanzaro@gnome.org>
Date: Thu, 25 Feb 2021 12:20:39 -0600
Subject: [PATCH 05/10] gspawn: Implement fd remapping for posix_spawn codepath
This means that GSubprocess will (sometimes) be able to use the
optimized posix_spawn codepath instead of having to fall back to
fork/exec.
---
glib/gspawn.c | 65 +++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 58 insertions(+), 7 deletions(-)
diff --git a/glib/gspawn.c b/glib/gspawn.c
index 149f5a5a1..301695980 100644
--- a/glib/gspawn.c
+++ b/glib/gspawn.c
@@ -1874,9 +1874,14 @@ 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;
@@ -1885,7 +1890,8 @@ do_posix_spawn (const gchar * const *argv,
GSList *child_close = NULL;
GSList *elem;
sigset_t mask;
- int i, r;
+ size_t i;
+ int r;
if (*argv[0] == '\0')
{
@@ -1999,6 +2005,43 @@ 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]);
+
+ 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);
+
+ 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.
*
@@ -2028,9 +2071,16 @@ do_posix_spawn (const gchar * const *argv,
*child_pid = pid;
out_close_fds:
- for (i = 0; i < num_parent_close_fds; i++)
+ for (i = 0; i < (size_t) 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);
@@ -2118,10 +2168,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",
@@ -2138,7 +2186,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;
--
2.33.1
From c3e147857c983835a22dbcad34928b40bfee3904 Mon Sep 17 00:00:00 2001
From: Michael Catanzaro <mcatanzaro@gnome.org>
Date: Thu, 25 Feb 2021 12:21:38 -0600
Subject: [PATCH 06/10] gsubprocess: ensure we test fd remapping on the
posix_spawn() codepath
We should run test_pass_fd twice, once using gspawn's fork/exec codepath
and once attempting to use its posix_spawn() codepath. There's no
guarantee we'll actually get the posix_spawn() codepath, but it works
for now on Linux.
For good measure, run it a third time with no flags at all.
This causes the test to fail if I separately break the fd remapping
implementation. Without this, we fail to test fd remapping on the
posix_spawn() codepath.
---
gio/tests/gsubprocess.c | 44 ++++++++++++++++++++++++++++++++++++++---
1 file changed, 41 insertions(+), 3 deletions(-)
diff --git a/gio/tests/gsubprocess.c b/gio/tests/gsubprocess.c
index 084b77df3..b10ade778 100644
--- a/gio/tests/gsubprocess.c
+++ b/gio/tests/gsubprocess.c
@@ -1697,7 +1697,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;
@@ -1722,9 +1723,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);
@@ -1754,6 +1757,39 @@ 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);
+}
+
#endif
static void
@@ -1890,7 +1926,9 @@ 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);
#endif
g_test_add_func ("/gsubprocess/launcher-environment", test_launcher_environment);
--
2.33.1
From ce174e40053d23f01bb13fd9453f0ffb8c2e7e2e Mon Sep 17 00:00:00 2001
From: Michael Catanzaro <mcatanzaro@redhat.com>
Date: Thu, 14 Oct 2021 11:01:33 -0500
Subject: [PATCH 07/10] gspawn: Check from errors from safe_dup2() and
dupfd_cloexec()
Although unlikely, these functions can fail, e.g. if we run out of file
descriptors. Check for errors to improve robustness. This is especially
important now that I changed our use of dupfd_cloexec() to avoid
returning fds smaller than the largest fd in target_fds. An application
that attempts to remap to the highest-allowed fd value deserves at least
some sort of attempt at error reporting, not silent failure.
---
glib/gspawn.c | 40 +++++++++++++++++++++++++++++-----------
1 file changed, 29 insertions(+), 11 deletions(-)
diff --git a/glib/gspawn.c b/glib/gspawn.c
index 301695980..848fd9f6e 100644
--- a/glib/gspawn.c
+++ b/glib/gspawn.c
@@ -1660,7 +1660,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);
@@ -1676,13 +1675,14 @@ do_exec (gint child_err_report_fd,
if (read_null < 0)
write_err_and_exit (child_err_report_fd,
CHILD_DUP2_FAILED);
- safe_dup2 (read_null, 0);
+ if (safe_dup2 (read_null, 0) < 0)
+ write_err_and_exit (child_err_report_fd,
+ CHILD_DUP2_FAILED);
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);
@@ -1697,13 +1697,14 @@ do_exec (gint child_err_report_fd,
if (write_null < 0)
write_err_and_exit (child_err_report_fd,
CHILD_DUP2_FAILED);
- safe_dup2 (write_null, 1);
+ if (safe_dup2 (write_null, 1) < 0)
+ write_err_and_exit (child_err_report_fd,
+ CHILD_DUP2_FAILED);
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);
@@ -1718,7 +1719,9 @@ do_exec (gint child_err_report_fd,
if (write_null < 0)
write_err_and_exit (child_err_report_fd,
CHILD_DUP2_FAILED);
- safe_dup2 (write_null, 2);
+ if (safe_dup2 (write_null, 2) < 0)
+ write_err_and_exit (child_err_report_fd,
+ CHILD_DUP2_FAILED);
close_and_invalidate (&write_null);
}
@@ -1731,7 +1734,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;
@@ -1770,7 +1774,11 @@ do_exec (gint child_err_report_fd,
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);
+ {
+ 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++)
@@ -1788,9 +1796,15 @@ do_exec (gint child_err_report_fd,
* dup it so it doesn’t get conflated.
*/
if (target_fds[i] == child_err_report_fd)
- child_err_report_fd = dupfd_cloexec (child_err_report_fd, max_target_fd + 1);
+ {
+ 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);
+ }
+
+ if (safe_dup2 (source_fds[i], target_fds[i]) < 0)
+ write_err_and_exit (child_err_report_fd, CHILD_DUP2_FAILED);
- safe_dup2 (source_fds[i], target_fds[i]);
close_and_invalidate (&source_fds[i]);
}
}
@@ -2033,7 +2047,11 @@ do_posix_spawn (const gchar * const *argv,
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);
+ {
+ 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++)
{
--
2.33.1
From 810b78fd79caa8c4418735f513921a5af1c225b3 Mon Sep 17 00:00:00 2001
From: Michael Catanzaro <mcatanzaro@redhat.com>
Date: Wed, 20 Oct 2021 16:51:44 -0500
Subject: [PATCH 08/10] gspawn: add new error message for open() failures
Reporting these as dup2() failures is bogus.
---
glib/gspawn.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/glib/gspawn.c b/glib/gspawn.c
index 848fd9f6e..dfd1b2268 100644
--- a/glib/gspawn.c
+++ b/glib/gspawn.c
@@ -1620,6 +1620,7 @@ enum
{
CHILD_CHDIR_FAILED,
CHILD_EXEC_FAILED,
+ CHILD_OPEN_FAILED,
CHILD_DUP2_FAILED,
CHILD_FORK_FAILED
};
@@ -1674,7 +1675,7 @@ do_exec (gint child_err_report_fd,
gint read_null = safe_open ("/dev/null", O_RDONLY);
if (read_null < 0)
write_err_and_exit (child_err_report_fd,
- CHILD_DUP2_FAILED);
+ CHILD_OPEN_FAILED);
if (safe_dup2 (read_null, 0) < 0)
write_err_and_exit (child_err_report_fd,
CHILD_DUP2_FAILED);
@@ -1696,7 +1697,7 @@ 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_DUP2_FAILED);
+ CHILD_OPEN_FAILED);
if (safe_dup2 (write_null, 1) < 0)
write_err_and_exit (child_err_report_fd,
CHILD_DUP2_FAILED);
@@ -1718,7 +1719,7 @@ 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_DUP2_FAILED);
+ CHILD_OPEN_FAILED);
if (safe_dup2 (write_null, 2) < 0)
write_err_and_exit (child_err_report_fd,
CHILD_DUP2_FAILED);
@@ -2508,7 +2509,15 @@ fork_exec (gboolean intermediate_child,
g_strerror (buf[1]));
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,
--
2.33.1
From 815b6f1de89cc665492af31893268fe58c686d6f Mon Sep 17 00:00:00 2001
From: Michael Catanzaro <mcatanzaro@redhat.com>
Date: Tue, 26 Oct 2021 21:27:15 -0500
Subject: [PATCH 09/10] Add tests for GSubprocess fd conflation issues
This tests for #2503. It's fragile, but there is no non-fragile way to
test this. If the test breaks in the future, it will pass without
successfully testing the bug, not fail spuriously, so I think this is
OK.
---
gio/tests/gsubprocess-testprog.c | 53 +++++++++++-
gio/tests/gsubprocess.c | 144 +++++++++++++++++++++++++++++++
2 files changed, 195 insertions(+), 2 deletions(-)
diff --git a/gio/tests/gsubprocess-testprog.c b/gio/tests/gsubprocess-testprog.c
index da3cf8d00..90be938b8 100644
--- a/gio/tests/gsubprocess-testprog.c
+++ b/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 expectedResult[] = "Yay success!";
+ guint8 buf[sizeof (expectedResult) + 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 (expectedResult))
+ {
+ g_warning ("Read %zu bytes, but expected %zu", bytes_read, sizeof (expectedResult));
+ return 1;
+ }
+
+ if (memcmp (expectedResult, buf, sizeof (expectedResult)) != 0)
+ {
+ buf[sizeof (expectedResult)] = '\0';
+ g_warning ("Expected \"%s\" but read \"%s\"", expectedResult, (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)
diff --git a/gio/tests/gsubprocess.c b/gio/tests/gsubprocess.c
index b10ade778..df13070f7 100644
--- a/gio/tests/gsubprocess.c
+++ b/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>
@@ -1790,6 +1791,146 @@ test_pass_fd_inherit_fds (void)
do_test_pass_fd (G_SUBPROCESS_FLAGS_INHERIT_FDS, NULL);
}
+static void
+do_test_fd_conflation (GSubprocessFlags flags,
+ GSpawnChildSetupFunc child_setup)
+{
+ char success_message[] = "Yay success!";
+ GError *error = NULL;
+ GOutputStream *output_stream;
+ GSubprocessLauncher *launcher;
+ GSubprocess *proc;
+ GPtrArray *args;
+ int unused_pipefds[2];
+ int pipefds[2];
+ 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.
+ *
+ * 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 the bugs
+ * it was originally designed to test. That is OK! If the test fails, at
+ * least you know *something* is wrong.
+ */
+ launcher = g_subprocess_launcher_new (flags);
+ g_subprocess_launcher_take_fd (launcher, pipefds[0] /* 5 */, pipefds[1] + 3 /* 9 */);
+ 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", pipefds[1] + 3);
+ 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]);
+
+ /* 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.
+ */
+ 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);
+}
+
+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);
+}
+
+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);
+}
+
#endif
static void
@@ -1929,6 +2070,9 @@ main (int argc, char **argv)
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);
#endif
g_test_add_func ("/gsubprocess/launcher-environment", test_launcher_environment);
--
2.33.1
From 44b4942a77e9f10a847bf06299d91491f7ad69ea Mon Sep 17 00:00:00 2001
From: Michael Catanzaro <mcatanzaro@redhat.com>
Date: Wed, 27 Oct 2021 18:30:47 -0500
Subject: [PATCH 10/10] Add test for child_err_report_fd conflation with target
fds
This tests for glib#2506.
---
gio/tests/gsubprocess.c | 42 ++++++++++++++++++++++++++++++++++-------
1 file changed, 35 insertions(+), 7 deletions(-)
diff --git a/gio/tests/gsubprocess.c b/gio/tests/gsubprocess.c
index df13070f7..6bc0aa4ea 100644
--- a/gio/tests/gsubprocess.c
+++ b/gio/tests/gsubprocess.c
@@ -1793,7 +1793,8 @@ test_pass_fd_inherit_fds (void)
static void
do_test_fd_conflation (GSubprocessFlags flags,
- GSpawnChildSetupFunc child_setup)
+ GSpawnChildSetupFunc child_setup,
+ gboolean test_child_err_report_fd)
{
char success_message[] = "Yay success!";
GError *error = NULL;
@@ -1803,6 +1804,7 @@ do_test_fd_conflation (GSubprocessFlags flags,
GPtrArray *args;
int unused_pipefds[2];
int pipefds[2];
+ int fd_to_pass_to_child;
gsize bytes_written;
gboolean success;
char *fd_str;
@@ -1855,18 +1857,26 @@ do_test_fd_conflation (GSubprocessFlags flags,
* 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 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 */, pipefds[1] + 3 /* 9 */);
+ 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", pipefds[1] + 3);
+ 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);
@@ -1882,12 +1892,20 @@ do_test_fd_conflation (GSubprocessFlags flags,
/* Also close the write end of the unused pipe. */
close (unused_pipefds[1]);
- /* So now pipefds[0] should be inherited into the subprocess as
+ /* 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,
@@ -1908,7 +1926,7 @@ do_test_fd_conflation (GSubprocessFlags flags,
static void
test_fd_conflation (void)
{
- do_test_fd_conflation (G_SUBPROCESS_FLAGS_NONE, NULL);
+ do_test_fd_conflation (G_SUBPROCESS_FLAGS_NONE, NULL, FALSE);
}
static void
@@ -1917,7 +1935,7 @@ 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);
+ do_test_fd_conflation (G_SUBPROCESS_FLAGS_NONE, empty_child_setup, FALSE);
}
static void
@@ -1928,7 +1946,16 @@ test_fd_conflation_inherit_fds (void)
* posix_spawn codepath does not currently handle closing
* non-inherited fds.
*/
- do_test_fd_conflation (G_SUBPROCESS_FLAGS_INHERIT_FDS, NULL);
+ 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
@@ -2073,6 +2100,7 @@ main (int argc, char **argv)
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);
--
2.33.1
Reference in New Issue View Git Blame Copy Permalink