[dconf/wip/reorg] engine: fix dconf_engine_unref() thread safety

[Ryan Lortie <ryanl src gnome org>]

commit a1b7bbb13507ba2381d78977c6c99d5aa0c1f1ba
Author: Ryan Lortie <desrt desrt ca>
Date:   Tue Jul 10 11:35:11 2012 -0400

    engine: fix dconf_engine_unref() thread safety
    
    There was a very slim race condition in the implementation of
    dconf_engine_unref().
    
    In order to check if the engine should be freed (to avoid double-freeing
    in two separate threads) the code was checking if the engine was still
    in the global list of engines.
    
    It is possible, however, that this thread could have unrefed the engine
    at exactly the same time as another thread, that thread won the race and
    freed the engine (removing it from the list) and then a third thread
    created another engine at the same time and it happened to have the same
    pointer address as the engine that was just freed.  In this case, the
    first engine would still see "itself" in the global list and free again.
    
    It's unlikely that this would ever happen in the real world but the
    regression tests picked it up.
    
    The new implementation should avoid that issue by not depending on the
    engine finding itself in the list as part of the decision about if it
    should be freed or not.

 engine/dconf-engine.c |   48 +++++++++++++++++-------------------------------
 1 files changed, 17 insertions(+), 31 deletions(-)
---
diff --git a/engine/dconf-engine.c b/engine/dconf-engine.c
index 24610ec..c1410b6 100644
--- a/engine/dconf-engine.c
+++ b/engine/dconf-engine.c
@@ -250,47 +250,30 @@ dconf_engine_new (gpointer       user_data,
 void
 dconf_engine_unref (DConfEngine *engine)
 {
-  if (g_atomic_int_dec_and_test (&engine->ref_count))
+  gint ref_count;
+
+ again:
+  ref_count = engine->ref_count;
+
+  if (ref_count == 1)
     {
       gint i;
 
-      /* We just dropped our refcount to zero, but we're still in the
-       * dconf_engine_global_list.
-       *
-       * If a signal arrives at this exact instant and the signal
-       * handler beats us to the lock then the refcount will be
-       * increased again.
+      /* We are about to drop the last reference, but there is a chance
+       * that a signal may be happening at this very moment, causing the
+       * engine to gain another reference (due to its position in the
+       * global engine list).
        *
-       * Acquire the lock and then double-check that the refcount is
-       * still zero before actually doing the remove.  If it's non-zero
-       * then the signal handler grabbed a ref and will call unref()
-       * later.
+       * Acquiring the lock here means that either we will remove this
+       * engine from the list first or we will notice the reference
+       * count has increased (and skip the free).
        */
       g_mutex_lock (&dconf_engine_global_lock);
-      if (g_atomic_int_get (&engine->ref_count) != 0)
-        {
-          g_mutex_unlock (&dconf_engine_global_lock);
-          return;
-        }
-
-      /* It's possible that another thread grabbed a reference at the
-       * last minute and dropped it back to zero again (thus causing the
-       * above check to pass).  In that case, however, the other thread
-       * will have also dropped the refcount from 1 to 0 and be inside
-       * of the dec-and-test above.
-       *
-       * We can only have one of the two threads doing the freeing of
-       * the data, so we have a simple rule: the thread that removes the
-       * engine from the global list is the one that does the free.
-       * Since this operation is performed under mutex we can be sure
-       * that only one thread will win.
-       */
-      if (!g_slist_find (dconf_engine_global_list, engine))
+      if (engine->ref_count != 1)
         {
           g_mutex_unlock (&dconf_engine_global_lock);
           return;
         }
-
       dconf_engine_global_list = g_slist_remove (dconf_engine_global_list, engine);
       g_mutex_unlock (&dconf_engine_global_lock);
 
@@ -309,6 +292,9 @@ dconf_engine_unref (DConfEngine *engine)
 
       g_slice_free (DConfEngine, engine);
     }
+
+  else if (!g_atomic_int_compare_and_exchange (&engine->ref_count, ref_count, ref_count - 1))
+    goto again;
 }
 
 static DConfEngine *