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.

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 *