[glib: 1/2] gspawn: Report errors with closing file descriptors between fork/exec

[Philip Withnall <pwithnall src gnome org>]

commit ce04a124040be091407e070280d86ca810bacb8c
Author: Philip Withnall <pwithnall endlessos org>
Date:   Mon Jan 17 15:27:24 2022 +0000

    gspawn: Report errors with closing file descriptors between fork/exec
    
    If a seccomp policy is set up incorrectly so that it returns `EPERM` for
    `close_range()` rather than `ENOSYS` due to it not being recognised, no
    error would previously be reported from GLib, but some file descriptors
    wouldn’t be closed, and that would cause a hung zombie process. The
    zombie process would be waiting for one half of a socket to be closed.
    
    Fix that by correctly propagating errors from `close_range()` back to the
    parent process so they can be reported correctly.
    
    Distributions which aren’t yet carrying the Docker fix to correctly
    return `ENOSYS` from unrecognised syscalls may want to temporarily carry
    an additional patch to fall back to `safe_fdwalk()` if `close_range()`
    fails with `EPERM`. This change will not be accepted upstream as `EPERM`
    is not the right error for `close_range()` to be returning.
    
    Signed-off-by: Philip Withnall <pwithnall endlessos org>
    
    Fixes: #2580

 glib/gspawn.c | 44 +++++++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 13 deletions(-)
---
diff --git a/glib/gspawn.c b/glib/gspawn.c
index 613c531e3..d4644f164 100644
--- a/glib/gspawn.c
+++ b/glib/gspawn.c
@@ -1520,7 +1520,7 @@ safe_fdwalk (int (*cb)(void *data, int fd), void *data)
 
 /* This function is called between fork() and exec() and hence must be
  * async-signal-safe (see signal-safety(7)). */
-static void
+static int
 safe_fdwalk_set_cloexec (int lowfd)
 {
 #if defined(HAVE_CLOSE_RANGE) && defined(CLOSE_RANGE_CLOEXEC)
@@ -1534,15 +1534,18 @@ safe_fdwalk_set_cloexec (int lowfd)
    * Handle ENOSYS in case it’s supported in libc but not the kernel; if so,
    * fall back to safe_fdwalk(). Handle EINVAL in case `CLOSE_RANGE_CLOEXEC`
    * is not supported. */
-  if (close_range (lowfd, G_MAXUINT, CLOSE_RANGE_CLOEXEC) != 0 &&
-      (errno == ENOSYS || errno == EINVAL))
+  int ret = close_range (lowfd, G_MAXUINT, CLOSE_RANGE_CLOEXEC);
+  if (ret == 0 || !(errno == ENOSYS || errno == EINVAL))
+    return ret;
 #endif  /* HAVE_CLOSE_RANGE */
-  (void) safe_fdwalk (set_cloexec, GINT_TO_POINTER (lowfd));
+  return safe_fdwalk (set_cloexec, GINT_TO_POINTER (lowfd));
 }
 
 /* This function is called between fork() and exec() and hence must be
- * async-signal-safe (see signal-safety(7)). */
-static void
+ * async-signal-safe (see signal-safety(7)).
+ *
+ * On failure, `-1` will be returned and errno will be set. */
+static int
 safe_closefrom (int lowfd)
 {
 #if defined(__FreeBSD__) || defined(__OpenBSD__) || \
@@ -1560,6 +1563,7 @@ safe_closefrom (int lowfd)
    * On such systems, F_CLOSEFROM is defined.
    */
   (void) closefrom (lowfd);
+  return 0;
 #elif defined(__DragonFly__)
   /* It is unclear whether closefrom function included in DragonFlyBSD libc_r
    * is safe to use because it calls a lot of library functions. It is also
@@ -1567,12 +1571,13 @@ safe_closefrom (int lowfd)
    * direct system call here ourselves to avoid possible issues.
    */
   (void) syscall (SYS_closefrom, lowfd);
+  return 0;
 #elif defined(F_CLOSEM)
   /* NetBSD and AIX have a special fcntl command which does the same thing as
    * closefrom. NetBSD also includes closefrom function, which seems to be a
    * simple wrapper of the fcntl command.
    */
-  (void) fcntl (lowfd, F_CLOSEM);
+  return fcntl (lowfd, F_CLOSEM);
 #else
 
 #if defined(HAVE_CLOSE_RANGE)
@@ -1582,9 +1587,11 @@ safe_closefrom (int lowfd)
    *
    * Handle ENOSYS in case it’s supported in libc but not the kernel; if so,
    * fall back to safe_fdwalk(). */
-  if (close_range (lowfd, G_MAXUINT, 0) != 0 && errno == ENOSYS)
+  int ret = close_range (lowfd, G_MAXUINT, 0);
+  if (ret == 0 || errno != ENOSYS)
+    return ret;
 #endif  /* HAVE_CLOSE_RANGE */
-  (void) safe_fdwalk (close_func, GINT_TO_POINTER (lowfd));
+  return safe_fdwalk (close_func, GINT_TO_POINTER (lowfd));
 #endif
 }
 
@@ -1622,7 +1629,8 @@ enum
   CHILD_EXEC_FAILED,
   CHILD_OPEN_FAILED,
   CHILD_DUP2_FAILED,
-  CHILD_FORK_FAILED
+  CHILD_FORK_FAILED,
+  CHILD_CLOSE_FAILED,
 };
 
 /* This function is called between fork() and exec() and hence must be
@@ -1738,12 +1746,14 @@ do_exec (gint                  child_err_report_fd,
           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);
+          if (safe_closefrom (4) < 0)
+            write_err_and_exit (child_err_report_fd, CHILD_CLOSE_FAILED);
           child_err_report_fd = 3;
         }
       else
         {
-          safe_fdwalk_set_cloexec (3);
+          if (safe_fdwalk_set_cloexec (3) < 0)
+            write_err_and_exit (child_err_report_fd, CHILD_CLOSE_FAILED);
         }
     }
   else
@@ -2543,7 +2553,15 @@ fork_exec (gboolean              intermediate_child,
                            _("Failed to fork child process (%s)"),
                            g_strerror (buf[1]));
               break;
-              
+
+            case CHILD_CLOSE_FAILED:
+              g_set_error (error,
+                           G_SPAWN_ERROR,
+                           G_SPAWN_ERROR_FAILED,
+                           _("Failed to close file descriptor for child process (%s)"),
+                           g_strerror (buf[1]));
+              break;
+
             default:
               g_set_error (error,
                            G_SPAWN_ERROR,