rpms/glib2
7
0
Fork 0
Code Issues Pull Requests Releases Activity
c9s
glib2/1968.patch
Michael Catanzaro d184827c6b WebKitGTK should use posix_spawn() to launch subprocesses 
This doesn't seem to be as important for Eclipse as originally expected,
but let's land it anyway. It notably fixes a couple nasty regressions
from RHEL 8, including one that could cause applications to hang when
launching a subprocess if they attempt to use the fd remapping
functionality.

Resolves: #1910092
2022-01-21 10:44:05 -06:00

1053 lines
38 KiB
Diff
Raw Permalink 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 9e69f8b280afe8eccd9188cc53b8117e1b238db7 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 a15fb1ca1..5d8422869 100644
--- a/glib/gspawn.c
+++ b/glib/gspawn.c
@@ -1710,7 +1710,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 fe2148fd5dd4f2e5c413c5cc0bb56c4a19304887 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 5d8422869..e214a3998 100644
--- a/glib/gspawn.c
+++ b/glib/gspawn.c
@@ -2363,7 +2363,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 566eccdb0a2594b4d3ec13c7443028d968b41af8 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 e214a3998..8bbe573f7 100644
--- a/glib/gspawn.c
+++ b/glib/gspawn.c
@@ -1500,20 +1500,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
@@ -1707,7 +1693,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 b703fa8b760ac9272c5a0ed3e3763b2f71ecf574 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 8bbe573f7..2b48b5600 100644
--- a/glib/gspawn.c
+++ b/glib/gspawn.c
@@ -1258,13 +1258,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);
@@ -1275,7 +1275,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);
@@ -1563,6 +1563,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,
@@ -1661,39 +1662,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 ecc3538a942760e8b403c319d359711c8e166778 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 2b48b5600..9ef78dbe1 100644
--- a/glib/gspawn.c
+++ b/glib/gspawn.c
@@ -1786,9 +1786,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;
@@ -1797,7 +1802,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')
{
@@ -1911,6 +1917,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.
*
@@ -1940,9 +1983,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);
@@ -2030,10 +2080,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",
@@ -2050,7 +2098,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 731d6c32105dc97f2b777ef9a34c6b76d1489c04 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 7e22678ec..ba49c1c43 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
@@ -1891,7 +1927,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 4608940466a04a32d4e6e71dbe872cfecb136118 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 9ef78dbe1..7ef678047 100644
--- a/glib/gspawn.c
+++ b/glib/gspawn.c
@@ -1572,7 +1572,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);
@@ -1588,13 +1587,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);
@@ -1609,13 +1609,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);
@@ -1630,7 +1631,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);
}
@@ -1643,7 +1646,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;
@@ -1682,7 +1686,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++)
@@ -1700,9 +1708,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]);
}
}
@@ -1945,7 +1959,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 0198b6a1c8c215f524d7c6ed2d240fb1b31d9865 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 7ef678047..c2fe306dc 100644
--- a/glib/gspawn.c
+++ b/glib/gspawn.c
@@ -1532,6 +1532,7 @@ enum
{
CHILD_CHDIR_FAILED,
CHILD_EXEC_FAILED,
+ CHILD_OPEN_FAILED,
CHILD_DUP2_FAILED,
CHILD_FORK_FAILED
};
@@ -1586,7 +1587,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);
@@ -1608,7 +1609,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);
@@ -1630,7 +1631,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);
@@ -2420,7 +2421,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 e4abb5f3db85b2f730e192e6398f26934e41ba21 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 c9b06c2a2..58cb1c71d 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 ba49c1c43..a6e24c2e8 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
@@ -1930,6 +2071,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 5542612c805857a244561ec160e904dd302ae799 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 a6e24c2e8..4629cdea7 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
@@ -2074,6 +2101,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