From 11cde2a4b9c3d538f62f7b1c49899f8d3f47a0e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Wed, 21 Jun 2023 16:59:55 +0100 Subject: [PATCH 1/2] glib: reset errno to 0 when futex() returns EAGAIN MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the GMutex backend that is based on futux() the g_mutex_lock() method will periodically clobber errno setting it to EAGAIN. This is because the futex usage pattern is inherantly racey (in a safe manner), such that we'll sometimes call futex() even if it is not required. In such cases futex() returns EAGAIN. Most of the time this is not a problem, as for the vast majority of APIs errno is irrelevant, and where it is relevant, it will only be examined if the function return value already indicated failure. The g_ascii_strto* functions are special, however, in that the return value cannot tell you if they failed or not. It is required to set errno to zero before calling them and check for non-zero errno afterwards. Unfortunately the g_ascii_* functions call a helper which uses mutexes, so when GMutex is built with futex, the g_ascii_* functions will sometimes end up returning with errno set to EAGAIN which the callers interpret as failure. To address this we must reset errno to its original value, if futex returns with errno == EAGAIN, as this is a success scenario. Since this is a race condition, reproducing it is somewhat non-deterministic. Thus for a test case we create a few threads and let them process 1000 mutex lock/unlock calls. This has been sufficient to detect the failure / validate the fix on local development machines. Closes: https://gitlab.gnome.org/GNOME/glib/-/issues/3034 Signed-off-by: Daniel P. Berrangé --- glib/gthreadprivate.h | 42 ++++++++++++++++++++++-------- glib/tests/mutex.c | 60 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 11 deletions(-) diff --git a/glib/gthreadprivate.h b/glib/gthreadprivate.h index 74d37ba322..a8cb984cef 100644 --- a/glib/gthreadprivate.h +++ b/glib/gthreadprivate.h @@ -54,6 +54,12 @@ struct _GRealThread /* Wrapper macro to call `futex_time64` and/or `futex` with simple * parameters and without returning the return value. * + * We expect futex to sometimes returns EAGAIN due to the race + * between the caller checking the current value and deciding to + * do the futex op. To avoid splattering errno on success, we + * restore the original errno if EAGAIN is seen. See also: + * https://gitlab.gnome.org/GNOME/glib/-/issues/3034 + * * If the `futex_time64` syscall does not exist (`ENOSYS`), we retry again * with the normal `futex` syscall. This can happen if newer kernel headers * are used than the kernel that is actually running. @@ -70,23 +76,37 @@ struct _GRealThread if (res < 0 && errno == ENOSYS) \ { \ errno = saved_errno; \ - syscall (__NR_futex, uaddr, (gsize) futex_op, __VA_ARGS__); \ + res = syscall (__NR_futex, uaddr, (gsize) futex_op, __VA_ARGS__); \ + } \ + if (res < 0 && errno == EAGAIN) \ + { \ + errno = saved_errno; \ } \ } \ G_STMT_END #elif defined(__NR_futex_time64) -#define g_futex_simple(uaddr, futex_op, ...) \ - G_STMT_START \ - { \ - syscall (__NR_futex_time64, uaddr, (gsize) futex_op, __VA_ARGS__); \ - } \ +#define g_futex_simple(uaddr, futex_op, ...) \ + G_STMT_START \ + { \ + int saved_errno = errno; \ + int res = syscall (__NR_futex_time64, uaddr, (gsize) futex_op, __VA_ARGS__); \ + if (res < 0 && errno == EAGAIN) \ + { \ + errno = saved_errno; \ + } \ + } \ G_STMT_END #elif defined(__NR_futex) -#define g_futex_simple(uaddr, futex_op, ...) \ - G_STMT_START \ - { \ - syscall (__NR_futex, uaddr, (gsize) futex_op, __VA_ARGS__); \ - } \ +#define g_futex_simple(uaddr, futex_op, ...) \ + G_STMT_START \ + { \ + int saved_errno = errno; \ + int res = syscall (__NR_futex, uaddr, (gsize) futex_op, __VA_ARGS__); \ + if (res < 0 && errno == EAGAIN) \ + { \ + errno = saved_errno; \ + } \ + } \ G_STMT_END #else /* !defined(__NR_futex) && !defined(__NR_futex_time64) */ #error "Neither __NR_futex nor __NR_futex_time64 are defined but were found by meson" diff --git a/glib/tests/mutex.c b/glib/tests/mutex.c index 2d0ef1ff3a..4a2e90fffe 100644 --- a/glib/tests/mutex.c +++ b/glib/tests/mutex.c @@ -159,6 +159,65 @@ test_mutex5 (void) g_assert (owners[i] == NULL); } +static gpointer +test_mutex_errno_func (gpointer data) +{ + GMutex *m = data; + int i; + + /* + * Validates that errno is not touched upon return + * https://gitlab.gnome.org/GNOME/glib/-/issues/3034 + */ + for (i = 0; i < 1000; i++) + { + errno = 0; + g_mutex_lock (m); + g_assert_cmpint (errno, ==, 0); + + g_thread_yield (); + + errno = 0; + g_mutex_unlock (m); + g_assert_cmpint (errno, ==, 0); + + errno = 0; + if (g_mutex_trylock (m)) + { + g_assert_cmpint (errno, ==, 0); + + g_thread_yield (); + + errno = 0; + g_mutex_unlock (m); + g_assert_cmpint (errno, ==, 0); + } + } + + return NULL; +} + +static void +test_mutex_errno (void) +{ + gsize i; + GThread *threads[THREADS]; + GMutex m; + + g_mutex_init (&m); + + for (i = 0; i < G_N_ELEMENTS (threads); i++) + { + threads[i] = g_thread_new ("test_mutex_errno", + test_mutex_errno_func, &m); + } + + for (i = 0; i < G_N_ELEMENTS (threads); i++) + { + g_thread_join (threads[i]); + } +} + static gint count_to = 0; static gboolean @@ -226,6 +285,7 @@ main (int argc, char *argv[]) g_test_add_func ("/thread/mutex3", test_mutex3); g_test_add_func ("/thread/mutex4", test_mutex4); g_test_add_func ("/thread/mutex5", test_mutex5); + g_test_add_func ("/thread/mutex/errno", test_mutex_errno); { guint i; -- GitLab From 3130879368816bfb91d7031f798a542005b2a3c4 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 22 Jun 2023 14:01:26 +0000 Subject: [PATCH 2/2] Apply 2 suggestion(s) to 2 file(s) --- glib/gthreadprivate.h | 2 +- glib/tests/mutex.c | 10 ++++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/glib/gthreadprivate.h b/glib/gthreadprivate.h index a8cb984cef..9de3d06ce5 100644 --- a/glib/gthreadprivate.h +++ b/glib/gthreadprivate.h @@ -54,7 +54,7 @@ struct _GRealThread /* Wrapper macro to call `futex_time64` and/or `futex` with simple * parameters and without returning the return value. * - * We expect futex to sometimes returns EAGAIN due to the race + * We expect futex to sometimes return EAGAIN due to the race * between the caller checking the current value and deciding to * do the futex op. To avoid splattering errno on success, we * restore the original errno if EAGAIN is seen. See also: diff --git a/glib/tests/mutex.c b/glib/tests/mutex.c index 4a2e90fffe..bd484c59e3 100644 --- a/glib/tests/mutex.c +++ b/glib/tests/mutex.c @@ -163,13 +163,11 @@ static gpointer test_mutex_errno_func (gpointer data) { GMutex *m = data; - int i; - /* - * Validates that errno is not touched upon return - * https://gitlab.gnome.org/GNOME/glib/-/issues/3034 - */ - for (i = 0; i < 1000; i++) + g_test_summary ("Validates that errno is not touched upon return"); + g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/3034"); + + for (unsigned int i = 0; i < 1000; i++) { errno = 0; g_mutex_lock (m); -- GitLab