From a434bfbb3d81382e6509054c32964934076d765c Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 17 Jan 2020 20:00:22 +0000 Subject: [PATCH] gdbusnameowning: Fix race between connection shutdown and NameLost MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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 ();