From f0e08e3488a5d33165ed864f935804ed2c573133 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 16 Nov 2022 12:37:22 +0000 Subject: [PATCH] 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));