gdbusnameowning: Fix race between connection shutdown and NameLost

As with all D-Bus signal subscriptions, it’s possible for a signal
callback to be invoked in one thread (T1) while another thread (T2) is
unsubscribing from that signal. In this case, T1 is the main thread, and
T2 is the D-Bus connection worker thread which is unsubscribing all
signals as it’s in the process of closing.

Due to this possibility, all `user_data` for signal callbacks needs to
be referenced outside the lifecycle of the code which
subscribes/unsubscribes the signal. In other words, it’s not safe to
subscribe to a signal, store the subscription ID in a struct,
unsubscribe from the signal when freeing the struct, and dereference the
struct in the signal callback. The data passed to the signal callback
has to have its own strong reference.

Instead, it’s safe to subscribe to a signal and add a strong reference
to the struct, store the subscription ID in that struct, and unsubscribe
from the signal when the last external reference to your struct is
dropped. That unsubscription should break the refcount cycle between the
signal connection and the struct, and allow the struct to be completely
freed. Only with that approach is it safe to dereference the struct in
the signal callback, if there’s any possibility that the signal might be
unsubscribed from a separate thread.

The tests need specific additional main loop cycles to completely emit
the NameLost signal callback. Ideally they need refactoring, but this
will do (1000 test cycles passed).

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Fixes: #978

gio/gdbusnameowning.c

@@ -366,7 +366,12 @@ request_name_cb (GObject      *source_object,
         connection = g_object_ref (client->connection);
       G_UNLOCK (lock);
 
-      /* start listening to NameLost and NameAcquired messages */
+      /* Start listening to NameLost and NameAcquired messages. We hold
+       * references to the Client in the signal closures, since it’s possible
+       * for a signal to be in-flight after unsubscribing the signal handler.
+       * This creates a reference count cycle, but that’s explicitly broken by
+       * disconnecting the signal handlers before calling client_unref() in
+       * g_bus_unown_name(). */
       if (connection != NULL)
         {
           client->name_lost_subscription_id =
@@ -378,8 +383,8 @@ request_name_cb (GObject      *source_object,
                                                 client->name,
                                                 G_DBUS_SIGNAL_FLAGS_NONE,
                                                 on_name_lost_or_acquired,
-                                                client,
+                                                client_ref (client),
-                                                NULL);
+                                                (GDestroyNotify) client_unref);
           client->name_acquired_subscription_id =
             g_dbus_connection_signal_subscribe (connection,
                                                 "org.freedesktop.DBus",
@@ -389,8 +394,8 @@ request_name_cb (GObject      *source_object,
                                                 client->name,
                                                 G_DBUS_SIGNAL_FLAGS_NONE,
                                                 on_name_lost_or_acquired,
-                                                client,
+                                                client_ref (client),
-                                                NULL);
+                                                (GDestroyNotify) client_unref);
           g_object_unref (connection);
         }
     }

gio/tests/gdbus-names.c

@@ -189,6 +189,7 @@ test_bus_own_name (void)
    * Stop owning the name - this should invoke our free func
    */
   g_bus_unown_name (id);
+  g_main_loop_run (loop);
   g_assert_cmpint (data.num_free_func, ==, 2);
 
   /*
@@ -330,6 +331,7 @@ test_bus_own_name (void)
   g_assert_cmpint (data2.num_acquired, ==, 0);
   g_assert_cmpint (data2.num_lost,     ==, 1);
   g_bus_unown_name (id2);
+  g_main_loop_run (loop);
   g_assert_cmpint (data2.num_bus_acquired, ==, 0);
   g_assert_cmpint (data2.num_acquired, ==, 0);
   g_assert_cmpint (data2.num_lost,     ==, 1);
@@ -355,6 +357,7 @@ test_bus_own_name (void)
   g_assert_cmpint (data2.num_acquired, ==, 0);
   g_assert_cmpint (data2.num_lost,     ==, 1);
   g_bus_unown_name (id2);
+  g_main_loop_run (loop);
   g_assert_cmpint (data2.num_bus_acquired, ==, 0);
   g_assert_cmpint (data2.num_acquired, ==, 0);
   g_assert_cmpint (data2.num_lost,     ==, 1);
@@ -365,6 +368,7 @@ test_bus_own_name (void)
    */
   data.expect_null_connection = FALSE;
   g_bus_unown_name (id);
+  g_main_loop_run (loop);
   g_assert_cmpint (data.num_bus_acquired, ==, 1);
   g_assert_cmpint (data.num_acquired, ==, 1);
   g_assert_cmpint (data.num_free_func, ==, 4);
@@ -418,6 +422,7 @@ test_bus_own_name (void)
   g_assert_cmpint (data2.num_acquired, ==, 0);
   g_assert_cmpint (data2.num_lost,     ==, 1);
   g_bus_unown_name (id2);
+  g_main_loop_run (loop);
   g_assert_cmpint (data2.num_bus_acquired, ==, 0);
   g_assert_cmpint (data2.num_acquired, ==, 0);
   g_assert_cmpint (data2.num_lost,     ==, 1);
@@ -450,8 +455,9 @@ test_bus_own_name (void)
   g_assert_cmpint (data2.num_bus_acquired, ==, 0);
   /* ok, make owner2 release the name - then wait for owner to automagically reacquire it */
   g_bus_unown_name (id2);
-  g_assert_cmpint (data2.num_free_func, ==, 1);
   g_main_loop_run (loop);
+  g_main_loop_run (loop);
+  g_assert_cmpint (data2.num_free_func, ==, 1);
   g_assert_cmpint (data.num_acquired, ==, 2);
   g_assert_cmpint (data.num_lost,     ==, 1);
 
@@ -466,6 +472,7 @@ test_bus_own_name (void)
   g_assert_cmpint (data.num_acquired, ==, 2);
   g_assert_cmpint (data.num_lost,     ==, 2);
   g_bus_unown_name (id);
+  g_main_loop_run (loop);
   g_assert_cmpint (data.num_free_func, ==, 5);
 
   g_object_unref (c);
@@ -645,6 +652,7 @@ test_bus_watch_name (void)
 
   /* unown the name */
   g_bus_unown_name (owner_id);
+  g_main_loop_run (loop);
   g_assert_cmpint (data.num_acquired, ==, 1);
   g_assert_cmpint (data.num_free_func, ==, 2);
 
@@ -704,6 +712,7 @@ test_bus_watch_name (void)
   g_assert_cmpint (data.num_free_func, ==, 1);
 
   g_bus_unown_name (owner_id);
+  g_main_loop_run (loop);
   g_assert_cmpint (data.num_free_func, ==, 2);
 
   session_bus_down ();