[glib: 2/3] gcancellable: Fix minor race between GCancellable and GCancellableSource

[Philip Withnall <pwithnall src gnome org>]

commit e4a690f5dd959e74b2d6054826f61509892c8aa7
Author: Philip Withnall <withnall endlessm com>
Date:   Fri Feb 21 14:44:44 2020 +0000

    gcancellable: Fix minor race between GCancellable and GCancellableSource
    
    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

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