From 4093321c9a831ae0be49e0aea70b6ef9ae906d46 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 28 Feb 2020 15:15:02 +0000 Subject: [PATCH 1/2] tests: Use g_assert_*() in cancellable test rather than g_assert() `g_assert()` is compiled out if `G_DISABLE_ASSERT` is defined, and `g_assert_*()` gives more detailed failure messages. Signed-off-by: Philip Withnall --- gio/tests/cancellable.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gio/tests/cancellable.c b/gio/tests/cancellable.c index cd349a8f3..4ba9f6326 100644 --- a/gio/tests/cancellable.c +++ b/gio/tests/cancellable.c @@ -138,7 +138,7 @@ mock_operation_finish (GAsyncResult *result, MockOperationData *data; GTask *task; - g_assert (g_task_is_valid (result, NULL)); + g_assert_true (g_task_is_valid (result, NULL)); /* This test expects the return value to be iterations_done even * when an error is set. @@ -212,7 +212,7 @@ test_cancel_multiple_concurrent (void) if (g_test_verbose ()) g_printerr ("CANCEL: %d operations\n", num_async_operations); g_cancellable_cancel (cancellable); - g_assert (g_cancellable_is_cancelled (cancellable)); + g_assert_true (g_cancellable_is_cancelled (cancellable)); /* Wait for all operations to be cancelled */ g_main_loop_run (loop); From e4a690f5dd959e74b2d6054826f61509892c8aa7 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 21 Feb 2020 14:44:44 +0000 Subject: [PATCH 2/2] gcancellable: Fix minor race between GCancellable and GCancellableSource MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There’s a minor race condition between cancellation of a `GCancellable`, and disposal/finalisation of a `GCancellableSource` in another thread. Thread A Thread B g_cancellable_cancel(C) →cancellable_source_cancelled(C, S) g_source_unref(S) cancellable_source_dispose(S) →→g_source_ref(S) →→# S is invalid at this point; crash Thankfully, the `GCancellable` sets `cancelled_running` while it’s emitting the `cancelled` signal, so if `cancellable_source_dispose()` is called while that’s high, we know that the thread which is doing the cancellation has already started (or is committed to starting) calling `cancellable_source_cancelled()`. Fix the race by resurrecting the `GCancellableSource` in `cancellable_source_dispose()`, and signalling this using `GCancellableSource.resurrected_during_cancellation`. Check for that flag in `cancellable_source_cancelled()` and ignore cancellation if it’s set. The modifications to `resurrected_during_cancellation` and the cancellable source’s refcount have to be done with `cancellable_mutex` held so that they are seen atomically by each thread. This should not affect performance too much, as it only happens during cancellation or disposal of a `GCancellableSource`. Signed-off-by: Philip Withnall Fixes: #1841 --- gio/gcancellable.c | 43 +++++++++++++++++++++++ gio/tests/cancellable.c | 77 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+) diff --git a/gio/gcancellable.c b/gio/gcancellable.c index d9e58b8e8..e687cca23 100644 --- a/gio/gcancellable.c +++ b/gio/gcancellable.c @@ -643,6 +643,8 @@ typedef struct { GCancellable *cancellable; gulong cancelled_handler; + /* Protected by cancellable_mutex: */ + gboolean resurrected_during_cancellation; } GCancellableSource; /* @@ -661,8 +663,24 @@ cancellable_source_cancelled (GCancellable *cancellable, gpointer user_data) { GSource *source = user_data; + GCancellableSource *cancellable_source = (GCancellableSource *) source; + + g_mutex_lock (&cancellable_mutex); + + /* Drop the reference added in cancellable_source_dispose(); see the comment there. + * The reference must be dropped after unlocking @cancellable_mutex since + * it could be the final reference, and the dispose function takes + * @cancellable_mutex. */ + if (cancellable_source->resurrected_during_cancellation) + { + cancellable_source->resurrected_during_cancellation = FALSE; + g_mutex_unlock (&cancellable_mutex); + g_source_unref (source); + return; + } g_source_ref (source); + g_mutex_unlock (&cancellable_mutex); g_source_set_ready_time (source, 0); g_source_unref (source); } @@ -684,12 +702,37 @@ cancellable_source_dispose (GSource *source) { GCancellableSource *cancellable_source = (GCancellableSource *)source; + g_mutex_lock (&cancellable_mutex); + if (cancellable_source->cancellable) { + if (cancellable_source->cancellable->priv->cancelled_running) + { + /* There can be a race here: if thread A has called + * g_cancellable_cancel() and has got as far as committing to call + * cancellable_source_cancelled(), then thread B drops the final + * ref on the GCancellableSource before g_source_ref() is called in + * cancellable_source_cancelled(), then cancellable_source_dispose() + * will run through and the GCancellableSource will be finalised + * before cancellable_source_cancelled() gets to g_source_ref(). It + * will then be left in a state where it’s committed to using a + * dangling GCancellableSource pointer. + * + * Eliminate that race by resurrecting the #GSource temporarily, and + * then dropping that reference in cancellable_source_cancelled(), + * which should be guaranteed to fire because we’re inside a + * @cancelled_running block. + */ + g_source_ref (source); + cancellable_source->resurrected_during_cancellation = TRUE; + } + g_clear_signal_handler (&cancellable_source->cancelled_handler, cancellable_source->cancellable); g_clear_object (&cancellable_source->cancellable); } + + g_mutex_unlock (&cancellable_mutex); } static gboolean diff --git a/gio/tests/cancellable.c b/gio/tests/cancellable.c index 4ba9f6326..002bdccca 100644 --- a/gio/tests/cancellable.c +++ b/gio/tests/cancellable.c @@ -228,6 +228,82 @@ test_cancel_null (void) g_cancellable_cancel (NULL); } +typedef struct +{ + GCond cond; + GMutex mutex; + GSource *cancellable_source; /* (owned) */ +} ThreadedDisposeData; + +static gboolean +cancelled_cb (GCancellable *cancellable, + gpointer user_data) +{ + /* Nothing needs to be done here. */ + return G_SOURCE_CONTINUE; +} + +static gpointer +threaded_dispose_thread_cb (gpointer user_data) +{ + ThreadedDisposeData *data = user_data; + + /* Synchronise with the main thread before trying to reproduce the race. */ + g_mutex_lock (&data->mutex); + g_cond_broadcast (&data->cond); + g_mutex_unlock (&data->mutex); + + /* Race with cancellation of the cancellable. */ + g_source_unref (data->cancellable_source); + + return NULL; +} + +static void +test_cancellable_source_threaded_dispose (void) +{ + guint i; + + g_test_summary ("Test a thread race between disposing of a GCancellableSource " + "(in one thread) and cancelling the GCancellable it refers " + "to (in another thread)"); + g_test_bug ("https://gitlab.gnome.org/GNOME/glib/issues/1841"); + + for (i = 0; i < 100000; i++) + { + GCancellable *cancellable = NULL; + GSource *cancellable_source = NULL; + ThreadedDisposeData data; + GThread *thread = NULL; + + /* Create a cancellable and a cancellable source for it. For this test, + * there’s no need to attach the source to a #GMainContext. */ + cancellable = g_cancellable_new (); + cancellable_source = g_cancellable_source_new (cancellable); + g_source_set_callback (cancellable_source, G_SOURCE_FUNC (cancelled_cb), NULL, NULL); + + /* Create a new thread and wait until it’s ready to execute before + * cancelling our cancellable. */ + g_cond_init (&data.cond); + g_mutex_init (&data.mutex); + data.cancellable_source = g_steal_pointer (&cancellable_source); + + g_mutex_lock (&data.mutex); + thread = g_thread_new ("/cancellable-source/threaded-dispose", + threaded_dispose_thread_cb, &data); + g_cond_wait (&data.cond, &data.mutex); + g_mutex_unlock (&data.mutex); + + /* Race with disposal of the cancellable source. */ + g_cancellable_cancel (cancellable); + + g_thread_join (g_steal_pointer (&thread)); + g_mutex_clear (&data.mutex); + g_cond_clear (&data.cond); + g_object_unref (cancellable); + } +} + int main (int argc, char *argv[]) { @@ -235,6 +311,7 @@ main (int argc, char *argv[]) g_test_add_func ("/cancellable/multiple-concurrent", test_cancel_multiple_concurrent); g_test_add_func ("/cancellable/null", test_cancel_null); + g_test_add_func ("/cancellable-source/threaded-dispose", test_cancellable_source_threaded_dispose); return g_test_run (); }