From d92f22ab47e145426dca0eec13fd198dc7ff82de Mon Sep 17 00:00:00 2001 From: Allison Karlitskaya Date: Fri, 28 Jun 2019 10:15:33 +0200 Subject: [PATCH 1/2] gthread: fix minor errno problem in GCond The return value from `g_cond_wait_until()` is calculated, based on the value of `errno` after reacquiring the mutex. This is a problem because `errno` can be overwritten in the case the mutex is contended (in which case the slow-path code will re-enter the kernel). Perform the calculation before reacquiring the mutex. See merge request GNOME/glib!958 --- glib/gthread-posix.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/glib/gthread-posix.c b/glib/gthread-posix.c index 8b8561639..c76f4433f 100644 --- a/glib/gthread-posix.c +++ b/glib/gthread-posix.c @@ -1441,6 +1441,7 @@ g_cond_wait_until (GCond *cond, struct timespec span; guint sampled; int res; + gboolean success; if (end_time < 0) return FALSE; @@ -1460,9 +1461,10 @@ g_cond_wait_until (GCond *cond, sampled = cond->i[0]; g_mutex_unlock (mutex); res = syscall (__NR_futex, &cond->i[0], (gsize) FUTEX_WAIT_PRIVATE, (gsize) sampled, &span); + success = (res < 0 && errno == ETIMEDOUT) ? FALSE : TRUE; g_mutex_lock (mutex); - return (res < 0 && errno == ETIMEDOUT) ? FALSE : TRUE; + return success; } #endif From de009c1e91984cdb95a95eb16360a3c8d1b42407 Mon Sep 17 00:00:00 2001 From: Allison Karlitskaya Date: Fri, 28 Jun 2019 10:15:42 +0200 Subject: [PATCH 2/2] tests: test g_cond_wait_until() under stress This (dubious) testcase fails before the previous commit due to errno being clobbered by the interrupted wait on the contended mutex. The previous commit fixes that. The testcase is dubious because, in theory (as per POSIX), g_cond_wait_until() is permitted to return TRUE at any time for any reason, due to so-called "spurious wakeups". Having a testcase that asserts that the return value should be FALSE is therefore fundamentally broken. We do it anyway, though. We're only really trying to test a bug in our homemade Linux/futex implementation here, and it takes a fair amount of effort to actually convince the old code to fail (including some system stuff which probably isn't available on Windows). There's also the spurious wakeup situation mentioned above to worry about on other systems. For all of those reasons, this test is only enabled on Linux. --- glib/tests/cond.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/glib/tests/cond.c b/glib/tests/cond.c index 0987f01e0..168064391 100644 --- a/glib/tests/cond.c +++ b/glib/tests/cond.c @@ -270,6 +270,100 @@ test_wait_until (void) g_cond_clear (&cond); } +#ifdef __linux__ + +#include +#include +#include + +static pthread_t main_thread; + +static void * +mutex_holder (void *data) +{ + GMutex *lock = data; + + g_mutex_lock (lock); + + /* Let the lock become contended */ + g_usleep (G_TIME_SPAN_SECOND); + + /* Interrupt the wait on the other thread */ + pthread_kill (main_thread, SIGHUP); + + /* If we don't sleep here, then the g_mutex_unlock() below will clear + * the mutex, causing the interrupted futex call in the other thread + * to return success (which is not what we want). + * + * The other thread needs to have time to wake up and see that the + * lock is still contended. + */ + g_usleep (G_TIME_SPAN_SECOND / 10); + + g_mutex_unlock (lock); + + return NULL; +} + +static void +signal_handler (int sig) +{ +} + +static void +test_wait_until_errno (void) +{ + gboolean result; + GMutex lock; + GCond cond; + + struct sigaction act = { { signal_handler, } }; /* important: no SA_RESTART (we want EINTR) */ + + g_test_summary ("Check proper handling of errno in g_cond_wait_until with a contended mutex"); + g_test_bug_base ("https://gitlab.gnome.org/GNOME/glib/"); + g_test_bug ("merge_requests/957"); + + g_mutex_init (&lock); + g_cond_init (&cond); + + main_thread = pthread_self (); + sigaction (SIGHUP, &act, NULL); + + g_mutex_lock (&lock); + + /* We create an annoying worker thread that will do two things: + * + * 1) hold the lock that we want to reacquire after returning from + * the condition variable wait + * + * 2) send us a signal to cause our wait on the contended lock to + * return EINTR, clobbering the errno return from the condition + * variable + */ + g_thread_unref (g_thread_new ("mutex-holder", mutex_holder, &lock)); + + result = g_cond_wait_until (&cond, &lock, + g_get_monotonic_time () + G_TIME_SPAN_SECOND / 50); + + /* Even after all that disruption, we should still successfully return + * 'timed out'. + */ + g_assert_false (result); + + g_mutex_unlock (&lock); + + g_cond_clear (&cond); + g_mutex_clear (&lock); +} + +#else +static void +test_wait_until_errno (void) +{ + g_test_skip ("We only test this on Linux"); +} +#endif + int main (int argc, char *argv[]) { @@ -278,6 +372,7 @@ main (int argc, char *argv[]) g_test_add_func ("/thread/cond1", test_cond1); g_test_add_func ("/thread/cond2", test_cond2); g_test_add_func ("/thread/cond/wait-until", test_wait_until); + g_test_add_func ("/thread/cond/wait-until/contended-and-interrupted", test_wait_until_errno); return g_test_run (); }