[glib: 1/2] GSubprocessLauncher: allow to close passed FDs

[Philip Withnall <pwithnall src gnome org>]

commit c12762a09158f843c36a84d652a163b986bf0ff8
Author: Sergio Costas <raster rastersoft com>
Date:   Sun Oct 4 14:34:41 2020 +0200

    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
 <SUBSECTION Standard>
 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 <unistd.h>
@@ -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 <glib-unix.h>
 #include <gio/gunixinputstream.h>
 #include <gio/gfiledescriptorbased.h>
+#include <unistd.h>
+#include <fcntl.h>
 #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);