[glib/glib-2-38] GSettingsBackend: fix a nasty race condition

[Ryan Lortie <desrt src gnome org>]

commit d5e172f8944fbca6b7df0aee0a4e7ef8d7912be8
Author: Ryan Lortie <desrt desrt ca>
Date:   Tue Mar 11 21:41:36 2014 -0400

    GSettingsBackend: fix a nasty race condition
    
    In the event that a GSettings object is being destroyed just as a change
    signal is being delivered, the destroying thread will race with the
    dconf worker thread for acquiring the lock on the GSettingsBackend.
    
    If the signalling thread gets there first then the destroying thread
    will block on the lock.  The signalling thread adds a reference to the
    GSettings object that is being destroyed and releases the lock.  The
    idea is that this should prevent the GSettings object from being
    destroyed and thus maintain its entry in the list.  Unfortunately, the
    weak reference notify function is already running and as soon as we
    release the lock, the list entry is removed.
    
    The signalling thread crashes.
    
    This bug is indicative of a serious problem encountered in many
    situations where GObject instances are touched from multiple threads.
    Ideally, we will move to a place where g_object_ref() is not called at
    all on the GSettings object from the dconf worker thread and instead, a
    dispatch will be done without holding a reference (similar to how
    GAppInfoMonitor presently works).  This would also prevent the
    unfortunate case of someone dropping what they assume to be the last
    reference on a GSettings object, only to have an already-pending signal
    delivered once they return to the mainloop, crashing their program.
    
    Making this change for GSettings (with multiple instances per thread,
    the possibility of multiple backends and each instance being interested
    in different events) is going to be extremely non-trivial, so it's not a
    change that makes sense at this point in the cycle.
    
    For now, we can do a relatively small and isolated tweak so that we
    never access the list except under a lock.  We still perform the bad
    pattern of acquiring a ref in a foreign thread which means that we still
    risk delivering a signal to a GSettings object that the user has assumed
    is dead (unless they explicitly disconnect their signal handler).  This
    is a problem that we already had, however.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=710367

 gio/gsettingsbackend.c |   44 ++++++++++++++++++++------------------------
 1 files changed, 20 insertions(+), 24 deletions(-)
---
diff --git a/gio/gsettingsbackend.c b/gio/gsettingsbackend.c
index 315d722..6517413 100644
--- a/gio/gsettingsbackend.c
+++ b/gio/gsettingsbackend.c
@@ -140,6 +140,7 @@ struct _GSettingsBackendClosure
                     gpointer           origin_tag,
                     gchar            **names);
 
+  GMainContext      *context;
   GObject           *target;
   GSettingsBackend  *backend;
   gchar             *name;
@@ -287,53 +288,48 @@ g_settings_backend_dispatch_signal (GSettingsBackend    *backend,
                                     gpointer             origin_tag,
                                     const gchar * const *names)
 {
-  GSettingsBackendWatch *suffix, *watch, *next;
+  GSettingsBackendWatch *watch;
+  GSList *closures = NULL;
 
   /* We're in a little bit of a tricky situation here.  We need to hold
    * a lock while traversing the list, but we don't want to hold the
    * lock while calling back into user code.
    *
-   * Since we're not holding the lock while we call user code, we can't
-   * render the list immutable.  We can, however, store a pointer to a
-   * given suffix of the list and render that suffix immutable.
-   *
-   * Adds will never modify the suffix since adds always come in the
-   * form of prepends.  We can also prevent removes from modifying the
-   * suffix since removes only happen in response to the last reference
-   * count dropping -- so just add a reference to everything in the
-   * suffix.
+   * We work around this by creating a bunch of GSettingsBackendClosure
+   * objects while holding the lock and dispatching them after.  We
+   * never touch the list without holding the lock.
    */
   g_mutex_lock (&backend->priv->lock);
-  suffix = backend->priv->watches;
-  for (watch = suffix; watch; watch = watch->next)
-    g_object_ref (watch->target);
-  g_mutex_unlock (&backend->priv->lock);
-
-  /* The suffix is now immutable, so this is safe. */
-  for (watch = suffix; watch; watch = next)
+  for (watch = backend->priv->watches; watch; watch = watch->next)
     {
       GSettingsBackendClosure *closure;
 
       closure = g_slice_new (GSettingsBackendClosure);
+      closure->context = watch->context;
       closure->backend = g_object_ref (backend);
-      closure->target = watch->target; /* we took our ref above */
+      closure->target = g_object_ref (watch->target);
       closure->function = G_STRUCT_MEMBER (void *, watch->vtable,
                                            function_offset);
       closure->name = g_strdup (name);
       closure->origin_tag = origin_tag;
       closure->names = g_strdupv ((gchar **) names);
 
-      /* we do this here because 'watch' may not live to the end of this
-       * iteration of the loop (since we may unref the target below).
-       */
-      next = watch->next;
+      closures = g_slist_prepend (closures, closure);
+    }
+  g_mutex_unlock (&backend->priv->lock);
 
-      if (watch->context)
-        g_main_context_invoke (watch->context,
+  while (closures)
+    {
+      GSettingsBackendClosure *closure = closures->data;
+
+      if (closure->context)
+        g_main_context_invoke (closure->context,
                                g_settings_backend_invoke_closure,
                                closure);
       else
         g_settings_backend_invoke_closure (closure);
+
+      closures = g_slist_delete_link (closures, closures);
     }
 }