public inbox for git-commits@fedoraproject.org
help / color / mirror / Atom feed
* [rpms/glib2] cve-2026-58016-f44: Add gspawn patchset
@ 2026-08-11 10:37 Michael Catanzaro
0 siblings, 0 replies; only message in thread
From: Michael Catanzaro @ 2026-08-11 10:37 UTC (permalink / raw)
To: git-commits
A new commit has been pushed.
Repo : rpms/glib2
Branch : cve-2026-58016-f44
Commit : 9849a2ff0ebbdd1a5829e94433809213f145fcf4
Author : Michael Catanzaro <mcatanzaro@redhat.com>
Date : 2021-12-06T09:58:23-06:00
Stats : +1056/-0 in 2 file(s)
URL : https://src.fedoraproject.org/rpms/glib2/c/9849a2ff0ebbdd1a5829e94433809213f145fcf4?branch=cve-2026-58016-f44
Log:
Add gspawn patchset
---
diff --git a/glib2.spec b/glib2.spec
index 058095f..e393ced 100644
--- a/glib2.spec
+++ b/glib2.spec
@@ -17,6 +17,10 @@ Patch0: gnutls-hmac.patch
# Proposed upstream at https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1596
Patch1: gdesktopappinfo.patch
+# https://bugzilla.redhat.com/show_bug.cgi?id=1910092
+# https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1968
+Patch2: gspawn.patch
+
BuildRequires: chrpath
BuildRequires: gcc
BuildRequires: gcc-c++
diff --git a/gspawn.patch b/gspawn.patch
new file mode 100644
index 0000000..8d0df57
--- /dev/null
+++ b/gspawn.patch
@@ -0,0 +1,1052 @@
+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
+
^ permalink raw reply related [flat|nested] only message in thread
only message in thread, other threads:[~2026-08-11 10:37 UTC | newest]
Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-08-11 10:37 [rpms/glib2] cve-2026-58016-f44: Add gspawn patchset Michael Catanzaro
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox