From f0e08e3488a5d33165ed864f935804ed2c573133 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 16 Nov 2022 12:37:22 +0000 Subject: [PATCH 1/2] gspawn: Ignore invalid FDs when using safe_fdwalk() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In `safe_closefrom()`, we thought it would be OK to assert that an FD being closed is valid, when using `safe_fdwalk()`, as it only walks over known-valid FDs. However, there is a situation where that might not be true: the program is being run under valgrind. Valgrind opens some FDs for its own use which are ≥1024, and it emulates a lowered soft limit on FDs. So if we were to use `safe_fdwalk_with_invalid_fds()` it would see the lowered soft limit and not try to close valgrind’s internal FDs. However, `safe_fdwalk()` looks at `/proc`, which valgrind does not emulate, so it sees the secret valgrind internal FDs, and then tries to close them. Valgrind doesn’t like this, prints ‘Warning: invalid file descriptor 1024 in syscall close()’ and returns `EBADF`. That return value causes `g_close()` to warn about faulty FD refcounting, and that causes unit test failures. Fix that by relaxing our assumptions about FD validity: use the `close_func_with_invalid_fds()` call back for closing FDs from `safe_fdwalk()`, rather than using `close_func()`. That will ignore `EBADF` return values. This should fix valgrind failures like this one: https://gitlab.gnome.org/GNOME/glib/-/jobs/2389977 Related prior art: https://bugs.freedesktop.org/show_bug.cgi?id=99839 Signed-off-by: Philip Withnall --- glib/gspawn.c | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/glib/gspawn.c b/glib/gspawn.c index b5a073163..be09f791b 100644 --- a/glib/gspawn.c +++ b/glib/gspawn.c @@ -1343,21 +1343,6 @@ dupfd_cloexec (int old_fd, int new_fd_min) return fd; } -/* fdwalk()-compatible callback to close a valid fd. - * It is an error to pass an invalid fd (causing EBADF) to this function. - * - * This function is called between fork() and exec() and hence must be - * async-signal-safe (see signal-safety(7)). - */ -G_GNUC_UNUSED static int -close_func (void *data, int fd) -{ - if (fd >= GPOINTER_TO_INT (data)) - g_close (fd, NULL); - - return 0; -} - /* fdwalk()-compatible callback to close a fd for non-compliant * implementations of fdwalk() that potentially pass already * closed fds. @@ -1678,7 +1663,7 @@ safe_closefrom (int lowfd) if (ret == 0 || errno != ENOSYS) return ret; #endif /* HAVE_CLOSE_RANGE */ - ret = safe_fdwalk (close_func, GINT_TO_POINTER (lowfd)); + ret = safe_fdwalk (close_func_with_invalid_fds, GINT_TO_POINTER (lowfd)); if (ret < 0 && errno == ENOSYS) ret = safe_fdwalk_with_invalid_fds (close_func_with_invalid_fds, GINT_TO_POINTER (lowfd)); From 3ee8dafb2797c43ec8844edded456f06da352494 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 22 Nov 2022 14:28:59 +0000 Subject: [PATCH 2/2] gspawn: Fold in safe_fdwalk_with_invalid_fds() helper function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since it’s now always called the same way after safe_fdwalk() has been called. This introduces no functional changes. Signed-off-by: Philip Withnall --- glib/gspawn.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/glib/gspawn.c b/glib/gspawn.c index be09f791b..f491c12cf 100644 --- a/glib/gspawn.c +++ b/glib/gspawn.c @@ -1412,6 +1412,8 @@ filename_to_fd (const char *p) } #endif +static int safe_fdwalk_with_invalid_fds (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 int @@ -1493,8 +1495,7 @@ safe_fdwalk (int (*cb)(void *data, int fd), void *data) return res; #endif - errno = ENOSYS; - return -1; + return safe_fdwalk_with_invalid_fds (cb, data); #endif } @@ -1605,9 +1606,6 @@ safe_fdwalk_set_cloexec (int lowfd) ret = safe_fdwalk (set_cloexec, GINT_TO_POINTER (lowfd)); - if (ret < 0 && errno == ENOSYS) - ret = safe_fdwalk_with_invalid_fds (set_cloexec, GINT_TO_POINTER (lowfd)); - return ret; } @@ -1665,9 +1663,6 @@ safe_closefrom (int lowfd) #endif /* HAVE_CLOSE_RANGE */ ret = safe_fdwalk (close_func_with_invalid_fds, GINT_TO_POINTER (lowfd)); - if (ret < 0 && errno == ENOSYS) - ret = safe_fdwalk_with_invalid_fds (close_func_with_invalid_fds, GINT_TO_POINTER (lowfd)); - return ret; #endif }