diff --git a/gcancellable-race-fix.patch b/gcancellable-race-fix.patch new file mode 100644 index 0000000..9e760d8 --- /dev/null +++ b/gcancellable-race-fix.patch @@ -0,0 +1,220 @@ +From e4a690f5dd959e74b2d6054826f61509892c8aa7 Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Fri, 21 Feb 2020 14:44:44 +0000 +Subject: [PATCH] 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 (); + } +-- +2.24.1 + diff --git a/glib2.spec b/glib2.spec index 4e463b3..a4944ee 100644 --- a/glib2.spec +++ b/glib2.spec @@ -2,13 +2,17 @@ Name: glib2 Version: 2.64.2 -Release: 1%{?dist} +Release: 2%{?dist} Summary: A library of handy utility functions License: LGPLv2+ URL: http://www.gtk.org Source0: http://download.gnome.org/sources/glib/2.64/glib-%{version}.tar.xz +# https://bugzilla.redhat.com/show_bug.cgi?id=1825230 +# https://gitlab.gnome.org/GNOME/glib/-/commit/e4a690f5dd959e74b2d6054826f61509892c8aa7 +Patch0: gcancellable-race-fix.patch + BuildRequires: chrpath BuildRequires: gcc BuildRequires: gcc-c++ @@ -215,6 +219,9 @@ glib-compile-schemas %{_datadir}/glib-2.0/schemas &> /dev/null || : %{_datadir}/installed-tests %changelog +* Tue Apr 28 2020 Tomas Popela - 2.64.2-2 +- Backport fix for a race condition in GCancellable (rhbz#1825230) + * Fri Apr 10 2020 Kalev Lember - 2.64.2-1 - Update to 2.64.2