From c12762a09158f843c36a84d652a163b986bf0ff8 Mon Sep 17 00:00:00 2001 From: Sergio Costas Date: Sun, 4 Oct 2020 14:34:41 +0200 Subject: [PATCH] GSubprocessLauncher: allow to close passed FDs By default, when using g_subprocess_launcher_take_fd() to pass an FD to a child, the GSubprocessLauncher object also takes ownership of the FD in the parent, and closes it during finalize(). This is a reasonable assumption in the majority of the cases, but sometimes it isn't a good idea. An example is when creating a GSubprocessLauncher in JavaScript: here, the destruction process is managed by the Garbage Collector, which means that those sockets will remain opened for some time after all the references to the object has been droped. This means that it could be not possible to detect when the child has closed that same FD, because in order to make that work, both FDs instances (the one in the parent and the one in the children) must be closed. This can be a problem in, as an example, a process that launches a child that communicates with Wayland using an specific socket (like when using the new API MetaWaylandClient). Of course, it isn't a valid solution to manually call close() in the parent process just after the call to spawn(), because the FD number could be reused in the time between it is manually closed, and when the object is destroyed and closes again that FD. If that happens, it will close an incorrect FD. One solution could be to call run_dispose() from Javascript on the GSubprocessLauncher object, to force freeing the resources. Unfortunately, the current code frees them in the finalize() method, not in dispose() (this is fixed in !1670 (merged) ) but it isn't a very elegant solution. This proposal adds a new method, g_subprocess_launcher_close(), that allows to close the FDs passed to the child. To avoid problems, after closing an FD with this method, no more spawns are allowed. Fix: https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1677 --- docs/reference/gio/gio-sections-common.txt | 1 + gio/gsubprocesslauncher-private.h | 1 + gio/gsubprocesslauncher.c | 91 +++++++++++++++------- gio/gsubprocesslauncher.h | 3 + gio/tests/gsubprocess.c | 32 ++++++++ 5 files changed, 102 insertions(+), 26 deletions(-) diff --git a/docs/reference/gio/gio-sections-common.txt b/docs/reference/gio/gio-sections-common.txt index 912c25507..3289bf6c6 100644 --- a/docs/reference/gio/gio-sections-common.txt +++ b/docs/reference/gio/gio-sections-common.txt @@ -4617,6 +4617,7 @@ g_subprocess_launcher_take_stdout_fd g_subprocess_launcher_set_stderr_file_path g_subprocess_launcher_take_stderr_fd g_subprocess_launcher_take_fd +g_subprocess_launcher_close g_subprocess_launcher_set_child_setup G_IS_SUBPROCESS_LAUNCHER diff --git a/gio/gsubprocesslauncher-private.h b/gio/gsubprocesslauncher-private.h index 64374a02d..95431a0ab 100644 --- a/gio/gsubprocesslauncher-private.h +++ b/gio/gsubprocesslauncher-private.h @@ -44,6 +44,7 @@ struct _GSubprocessLauncher GArray *basic_fd_assignments; GArray *needdup_fd_assignments; + gboolean closed_fd; GSpawnChildSetupFunc child_setup_func; gpointer child_setup_user_data; diff --git a/gio/gsubprocesslauncher.c b/gio/gsubprocesslauncher.c index 4f9bbaa9a..c27960949 100644 --- a/gio/gsubprocesslauncher.c +++ b/gio/gsubprocesslauncher.c @@ -46,6 +46,7 @@ #include "gioenumtypes.h" #include "gsubprocess.h" #include "ginitable.h" +#include "gioerror.h" #ifdef G_OS_UNIX #include @@ -136,36 +137,11 @@ g_subprocess_launcher_dispose (GObject *object) GSubprocessLauncher *self = G_SUBPROCESS_LAUNCHER (object); #ifdef G_OS_UNIX - guint i; - g_clear_pointer (&self->stdin_path, g_free); g_clear_pointer (&self->stdout_path, g_free); g_clear_pointer (&self->stderr_path, g_free); - if (self->stdin_fd != -1) - close (self->stdin_fd); - self->stdin_fd = -1; - - if (self->stdout_fd != -1) - close (self->stdout_fd); - self->stdout_fd = -1; - - if (self->stderr_fd != -1) - close (self->stderr_fd); - self->stderr_fd = -1; - - if (self->basic_fd_assignments) - { - for (i = 0; i < self->basic_fd_assignments->len; i++) - (void) close (g_array_index (self->basic_fd_assignments, int, i)); - g_clear_pointer (&self->basic_fd_assignments, g_array_unref); - } - if (self->needdup_fd_assignments) - { - for (i = 0; i < self->needdup_fd_assignments->len; i += 2) - (void) close (g_array_index (self->needdup_fd_assignments, int, i)); - g_clear_pointer (&self->needdup_fd_assignments, g_array_unref); - } + g_subprocess_launcher_close (self); if (self->child_setup_destroy_notify) (* self->child_setup_destroy_notify) (self->child_setup_user_data); @@ -652,6 +628,58 @@ g_subprocess_launcher_take_fd (GSubprocessLauncher *self, } } +/** + * g_subprocess_launcher_close: + * @self: a #GSubprocessLauncher + * + * Closes all the file descriptors previously passed to the object with + * g_subprocess_launcher_take_fd(), g_subprocess_launcher_take_stderr_fd(), etc. + * + * After calling this method, any subsequent calls to g_subprocess_launcher_spawn() or g_subprocess_launcher_spawnv() will + * return %G_IO_ERROR_CLOSED. This method is idempotent if + * called more than once. + * + * This function is called automatically when the #GSubprocessLauncher + * is disposed, but is provided separately so that garbage collected + * language bindings can call it earlier to guarantee when FDs are closed. + * + * Since: 2.68 + */ +void +g_subprocess_launcher_close (GSubprocessLauncher *self) +{ + guint i; + + g_return_if_fail (G_IS_SUBPROCESS_LAUNCHER (self)); + + if (self->stdin_fd != -1) + close (self->stdin_fd); + self->stdin_fd = -1; + + if (self->stdout_fd != -1) + close (self->stdout_fd); + self->stdout_fd = -1; + + if (self->stderr_fd != -1) + close (self->stderr_fd); + self->stderr_fd = -1; + + if (self->basic_fd_assignments) + { + for (i = 0; i < self->basic_fd_assignments->len; i++) + (void) close (g_array_index (self->basic_fd_assignments, int, i)); + g_clear_pointer (&self->basic_fd_assignments, g_array_unref); + } + if (self->needdup_fd_assignments) + { + for (i = 0; i < self->needdup_fd_assignments->len; i += 2) + (void) close (g_array_index (self->needdup_fd_assignments, int, i)); + g_clear_pointer (&self->needdup_fd_assignments, g_array_unref); + } + + self->closed_fd = TRUE; +} + /** * g_subprocess_launcher_set_child_setup: (skip) * @self: a #GSubprocessLauncher @@ -754,6 +782,17 @@ g_subprocess_launcher_spawnv (GSubprocessLauncher *launcher, g_return_val_if_fail (argv != NULL && argv[0] != NULL && argv[0][0] != '\0', NULL); +#ifdef G_OS_UNIX + if (launcher->closed_fd) + { + g_set_error (error, + G_IO_ERROR, + G_IO_ERROR_CLOSED, + "Can't spawn a new child because a passed file descriptor has been closed."); + return NULL; + } +#endif + subprocess = g_object_new (G_TYPE_SUBPROCESS, "argv", argv, "flags", launcher->flags, diff --git a/gio/gsubprocesslauncher.h b/gio/gsubprocesslauncher.h index 05d83f131..0654c2b70 100644 --- a/gio/gsubprocesslauncher.h +++ b/gio/gsubprocesslauncher.h @@ -103,6 +103,9 @@ void g_subprocess_launcher_take_fd (GSubpro gint source_fd, gint target_fd); +GLIB_AVAILABLE_IN_2_68 +void g_subprocess_launcher_close (GSubprocessLauncher *self); + /* Child setup, only available on UNIX */ GLIB_AVAILABLE_IN_2_40 void g_subprocess_launcher_set_child_setup (GSubprocessLauncher *self, diff --git a/gio/tests/gsubprocess.c b/gio/tests/gsubprocess.c index bccf6a308..3c248e610 100644 --- a/gio/tests/gsubprocess.c +++ b/gio/tests/gsubprocess.c @@ -6,6 +6,8 @@ #include #include #include +#include +#include #endif /* We write 2^1 + 2^2 ... + 2^10 or 2047 copies of "Hello World!\n" @@ -1483,6 +1485,35 @@ test_cwd (void) g_object_unref (launcher); } #ifdef G_OS_UNIX + +static void +test_subprocess_launcher_close (void) +{ + GError *local_error = NULL; + GError **error = &local_error; + GSubprocessLauncher *launcher; + GSubprocess *proc; + GPtrArray *args; + int fd; + gboolean is_open; + + fd = dup(0); + launcher = g_subprocess_launcher_new (G_SUBPROCESS_FLAGS_NONE); + g_subprocess_launcher_take_fd (launcher, fd, fd); + is_open = fcntl (fd, F_GETFD) != -1; + g_assert_true (is_open); + g_subprocess_launcher_close (launcher); + is_open = fcntl (fd, F_GETFD) != -1; + g_assert_false (is_open); + args = get_test_subprocess_args ("cat", NULL); + proc = g_subprocess_launcher_spawnv (launcher, (const gchar * const *) args->pdata, error); + g_ptr_array_free (args, TRUE); + g_assert_null (proc); + g_assert_error (local_error, G_IO_ERROR, G_IO_ERROR_CLOSED); + g_clear_error (error); + g_object_unref (launcher); +} + static void test_stdout_file (void) { @@ -1835,6 +1866,7 @@ main (int argc, char **argv) g_test_add_func ("/gsubprocess/env/inherit", test_env_inherit); g_test_add_func ("/gsubprocess/cwd", test_cwd); #ifdef G_OS_UNIX + g_test_add_func ("/gsubprocess/launcher-close", test_subprocess_launcher_close); 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);