luc14n0/glib
1
0
Fork 0
mirror of https://gitlab.gnome.org/GNOME/glib.git synced 2024-12-25 15:06:14 +01:00
Code Issues Packages Projects Releases Wiki Activity

gcancellable: Fix minor race between GCancellable and GCancellableSource

Browse Source 
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 <withnall@endlessm.com>

Fixes: #1841
This commit is contained in:
Philip Withnall 2020-02-21 14:44:44 +00:00
parent 4093321c9a
commit e4a690f5dd
2 changed files with 120 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

43
gio/gcancellable.c
View File

@ -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

77
gio/tests/cancellable.c
View File

@ -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 ();
}