[glib: 6/8] gdbusnameowning: Fix race between connection shutdown and NameLost

[Philip Withnall <pwithnall src gnome org>]

commit a434bfbb3d81382e6509054c32964934076d765c
Author: Philip Withnall <withnall endlessm com>
Date:   Fri Jan 17 20:00:22 2020 +0000

    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   | 15 ++++++++++-----
 gio/tests/gdbus-names.c | 11 ++++++++++-
 2 files changed, 20 insertions(+), 6 deletions(-)
---
diff --git a/gio/gdbusnameowning.c b/gio/gdbusnameowning.c
index 9764f2476..839a88cfb 100644
--- a/gio/gdbusnameowning.c
+++ b/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,
-                                                NULL);
+                                                client_ref (client),
+                                                (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,
-                                                NULL);
+                                                client_ref (client),
+                                                (GDestroyNotify) client_unref);
           g_object_unref (connection);
         }
     }
diff --git a/gio/tests/gdbus-names.c b/gio/tests/gdbus-names.c
index 4f95b457a..94c777a30 100644
--- a/gio/tests/gdbus-names.c
+++ b/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 ();