From f0bcb7f79c865a3cdd6f41b7e08f67a1a3d9d55c 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 0cfc5b054a6a42d53bc7aa255785206b78c71084 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. Backport 2.74: Modified to drop documentation changes to g_close() which document its new async-signal-safe guarantees. They are not public guarantees until 2.76. Also modified to include moving the code to ignore `EINTR` from commit d5dc7d266f2b8d0f7d. --- glib/gstdio.c | 47 +++++++++++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/glib/gstdio.c b/glib/gstdio.c index f2d58134e..fa291c287 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 @@ -1766,24 +1767,38 @@ g_close (gint fd, GError **error) { int res; - res = close (fd); - /* Just ignore EINTR for now; a retry loop is the wrong thing to do - * on Linux at least. Anyone who wants to add a conditional check - * for e.g. HP-UX is welcome to do so later... - * - * http://lkml.indiana.edu/hypermail/linux/kernel/0509.1/0877.html - * https://bugzilla.gnome.org/show_bug.cgi?id=682819 - * http://utcc.utoronto.ca/~cks/space/blog/unix/CloseEINTR - * https://sites.google.com/site/michaelsafyan/software-engineering/checkforeintrwheninvokingclosethinkagain + + /* Important: if @error is NULL, we must not do anything that is + * not async-signal-safe. */ - if (G_UNLIKELY (res == -1 && errno == EINTR)) - return TRUE; - else if (res == -1) + res = close (fd); + + if (res == -1) { int errsv = errno; - g_set_error_literal (error, G_FILE_ERROR, - g_file_error_from_errno (errsv), - g_strerror (errsv)); + + if (errsv == EINTR) + { + /* Just ignore EINTR for now; a retry loop is the wrong thing to do + * on Linux at least. Anyone who wants to add a conditional check + * for e.g. HP-UX is welcome to do so later... + * + * https://lwn.net/Articles/576478/ + * http://lkml.indiana.edu/hypermail/linux/kernel/0509.1/0877.html + * https://bugzilla.gnome.org/show_bug.cgi?id=682819 + * http://utcc.utoronto.ca/~cks/space/blog/unix/CloseEINTR + * https://sites.google.com/site/michaelsafyan/software-engineering/checkforeintrwheninvokingclosethinkagain + */ + return TRUE; + } + + if (error) + { + g_set_error_literal (error, G_FILE_ERROR, + g_file_error_from_errno (errsv), + g_strerror (errsv)); + } + errno = errsv; return FALSE; } From 7720c598f4a8b27d975f1e5081b01865bfd43ec9 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; }