From ce04a124040be091407e070280d86ca810bacb8c Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 17 Jan 2022 15:27:24 +0000 Subject: [PATCH] gspawn: Report errors with closing file descriptors between fork/exec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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,