From 6418db829b6f56b92beb044f51f94c7a1bd7ae51 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 12 Oct 2022 23:30:37 +0200 Subject: [PATCH 1/3] gspawn: avoid race due to retry with EINTR on close() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Retry on EINTR is wrong on many OS, including Linux. See the comment in g_close() why that is. As we cannot use g_close() after fork, we had safe_close(). This had the wrong retry loop on EINTR. Drop that. This was especially problematic since commit 6f46294227f8 ('gspawn: Don’t use g_close() in async-signal-safe context'). Before, safe_close() was only called after fork, where there is only one thread and there is no concern about a race. This patch only exists for easier backporting of the bugfix. The code will be reworked further next. Fixes: 6f46294227f8 ('gspawn: Don’t use g_close() in async-signal-safe context') --- glib/gspawn.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/glib/gspawn.c b/glib/gspawn.c index 4e029eedf..c644cebb1 100644 --- a/glib/gspawn.c +++ b/glib/gspawn.c @@ -162,7 +162,7 @@ extern char **environ; */ -static gint safe_close (gint fd); +static void safe_close (gint fd); static gint g_execute (const gchar *file, gchar **argv, @@ -1340,16 +1340,21 @@ dupfd_cloexec (int old_fd, int new_fd_min) /* This function is called between fork() and exec() and hence must be * async-signal-safe (see signal-safety(7)). */ -static gint +static void safe_close (gint fd) { - gint ret; - - do - ret = close (fd); - while (ret < 0 && errno == EINTR); - - return ret; + /* Note that this function is (also) called after fork(), so it cannot use + * g_close(). + * Note that it is however called both from the parent and the child + * process. + * + * This function returns no error, because there is nothing what the caller + * could do with that information. That is even the case for EINTR. See + * g_close() about the specialty of EINTR and why that is correct. + * If g_close() ever gets extended to handle EINTR specially, then this place + * and all other direct calls to close() need updating. + */ + close (fd); } /* This function is called between fork() and exec() and hence must be @@ -1358,7 +1363,7 @@ G_GNUC_UNUSED static int close_func (void *data, int fd) { if (fd >= GPOINTER_TO_INT (data)) - (void) safe_close (fd); + safe_close (fd); return 0; } From 3ed7f4d1ed7e49b5d38d837d3edba69f5b7eaaf6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Oct 2022 09:07:35 +0200 Subject: [PATCH 2/3] gstdio: make g_close() async-signal-safe under certain conditions g_close() does something useful. It is not trivial to get EINTR handling of close() right, in a portable manner. g_close() abstracts this. We should allow glib users to use the function even in async-signal-safe contexts, at least if the user heeds the caveat about GError and take care not to fail assertions. --- glib/gstdio.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/glib/gstdio.c b/glib/gstdio.c index 22d1159ce..7fe7b32ed 100644 --- a/glib/gstdio.c +++ b/glib/gstdio.c @@ -1749,8 +1749,9 @@ g_utime (const gchar *filename, * @fd: A file descriptor * @error: a #GError * - * This wraps the close() call; in case of error, %errno will be + * This wraps the close() call. In case of error, %errno will be * preserved, but the error will also be stored as a #GError in @error. + * In case of success, %errno is undefined. * * Besides using #GError, there is another major reason to prefer this * function over the call provided by the system; on Unix, it will @@ -1759,6 +1760,9 @@ g_utime (const gchar *filename, * * It is a bug to call this function with an invalid file descriptor. * + * Since 2.76, this function is guaranteed to be async-signal-safe if (and only + * if) @error is %NULL and @fd is a valid open file descriptor. + * * Returns: %TRUE on success, %FALSE if there was an error. * * Since: 2.36 @@ -1769,6 +1773,9 @@ g_close (gint fd, { int res; + /* Important: if @error is NULL, we must not do anything that is + * not async-signal-safe. + */ res = close (fd); if (res == -1) @@ -1790,12 +1797,17 @@ g_close (gint fd, return TRUE; } - g_set_error_literal (error, G_FILE_ERROR, - g_file_error_from_errno (errsv), - g_strerror (errsv)); + if (error) + { + g_set_error_literal (error, G_FILE_ERROR, + g_file_error_from_errno (errsv), + g_strerror (errsv)); + } if (errsv == EBADF) { + /* There is a bug. Fail an assertion. Note that this function is supposed to be + * async-signal-safe, but in case an assertion fails, all bets are already off. */ if (fd >= 0) { /* Closing an non-negative, invalid file descriptor is a bug. The bug is From d6790aeccaf7c680c29a34f8094fa098038e8499 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 12 Oct 2022 23:30:37 +0200 Subject: [PATCH 3/3] gspawn: use g_close() g_close() now is async-signal-safe, as long as we don't request a GError and pass a valid file descriptor. Update "gspawn.c" to drop its safe_close() function and use g_close() instead. --- glib/gspawn.c | 33 +++++---------------------------- 1 file changed, 5 insertions(+), 28 deletions(-) diff --git a/glib/gspawn.c b/glib/gspawn.c index c644cebb1..4f4c55d99 100644 --- a/glib/gspawn.c +++ b/glib/gspawn.c @@ -162,8 +162,6 @@ extern char **environ; */ -static void safe_close (gint fd); - static gint g_execute (const gchar *file, gchar **argv, gchar **argv_buffer, @@ -267,11 +265,9 @@ close_and_invalidate (gint *fd) { if (*fd < 0) return; - else - { - safe_close (*fd); - *fd = -1; - } + + g_close (*fd, NULL); + *fd = -1; } /* Some versions of OS X define READ_OK in public headers */ @@ -1338,32 +1334,13 @@ dupfd_cloexec (int old_fd, int new_fd_min) return fd; } -/* This function is called between fork() and exec() and hence must be - * async-signal-safe (see signal-safety(7)). */ -static void -safe_close (gint fd) -{ - /* Note that this function is (also) called after fork(), so it cannot use - * g_close(). - * Note that it is however called both from the parent and the child - * process. - * - * This function returns no error, because there is nothing what the caller - * could do with that information. That is even the case for EINTR. See - * g_close() about the specialty of EINTR and why that is correct. - * If g_close() ever gets extended to handle EINTR specially, then this place - * and all other direct calls to close() need updating. - */ - close (fd); -} - /* 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)) - safe_close (fd); + g_close (fd, NULL); return 0; } @@ -1458,7 +1435,7 @@ safe_fdwalk (int (*cb)(void *data, int fd), void *data) } } - safe_close (dir_fd); + g_close (dir_fd, NULL); return res; }