From: David Zeuthen Date: Thu, 27 Oct 2011 14:30:58 +0000 (-0400) Subject: g_bus_own_name: fix race when unowning a name immediately after owning it X-Git-Tag: 2.31.2~161 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=1fc897352e2bd8c52f33517088213ee4b0024932;p=platform%2Fupstream%2Fglib.git g_bus_own_name: fix race when unowning a name immediately after owning it ... and also add a test to verify that the fix works. https://bugzilla.gnome.org/show_bug.cgi?id=662808 Signed-off-by: David Zeuthen --- diff --git a/gio/gdbusnameowning.c b/gio/gdbusnameowning.c index 65b1227..207fa60 100644 --- a/gio/gdbusnameowning.c +++ b/gio/gdbusnameowning.c @@ -74,7 +74,7 @@ typedef struct guint name_acquired_subscription_id; guint name_lost_subscription_id; - gboolean cancelled; + volatile gboolean cancelled; /* must hold lock when reading or modifying */ gboolean needs_release; } Client; @@ -219,27 +219,39 @@ do_call (Client *client, CallType call_type) static void call_acquired_handler (Client *client) { + G_LOCK (lock); if (client->previous_call != PREVIOUS_CALL_ACQUIRED) { client->previous_call = PREVIOUS_CALL_ACQUIRED; if (!client->cancelled) { + G_UNLOCK (lock); do_call (client, CALL_TYPE_NAME_ACQUIRED); + goto out; } } + G_UNLOCK (lock); + out: + ; } static void call_lost_handler (Client *client) { + G_LOCK (lock); if (client->previous_call != PREVIOUS_CALL_LOST) { client->previous_call = PREVIOUS_CALL_LOST; if (!client->cancelled) { + G_UNLOCK (lock); do_call (client, CALL_TYPE_NAME_LOST); + goto out; } } + G_UNLOCK (lock); + out: + ; } /* ---------------------------------------------------------------------------------------------------- */ @@ -296,7 +308,8 @@ request_name_cb (GObject *source_object, request_name_reply = 0; result = NULL; - result = g_dbus_connection_call_finish (client->connection, + /* don't use client->connection - it may be NULL already */ + result = g_dbus_connection_call_finish (G_DBUS_CONNECTION (source_object), res, NULL); if (result != NULL) @@ -332,31 +345,47 @@ request_name_cb (GObject *source_object, break; } + if (subscribe) { + GDBusConnection *connection = NULL; + + /* if cancelled, there is no point in subscribing to signals - if not, make sure + * we use a known good Connection object since it may be set to NULL at any point + * after being cancelled + */ + G_LOCK (lock); + if (!client->cancelled) + connection = g_object_ref (client->connection); + G_UNLOCK (lock); + /* start listening to NameLost and NameAcquired messages */ - client->name_lost_subscription_id = - g_dbus_connection_signal_subscribe (client->connection, - "org.freedesktop.DBus", - "org.freedesktop.DBus", - "NameLost", - "/org/freedesktop/DBus", - client->name, - G_DBUS_SIGNAL_FLAGS_NONE, - on_name_lost_or_acquired, - client, - NULL); - client->name_acquired_subscription_id = - g_dbus_connection_signal_subscribe (client->connection, - "org.freedesktop.DBus", - "org.freedesktop.DBus", - "NameAcquired", - "/org/freedesktop/DBus", - client->name, - G_DBUS_SIGNAL_FLAGS_NONE, - on_name_lost_or_acquired, - client, - NULL); + if (connection != NULL) + { + client->name_lost_subscription_id = + g_dbus_connection_signal_subscribe (connection, + "org.freedesktop.DBus", + "org.freedesktop.DBus", + "NameLost", + "/org/freedesktop/DBus", + client->name, + G_DBUS_SIGNAL_FLAGS_NONE, + on_name_lost_or_acquired, + client, + NULL); + client->name_acquired_subscription_id = + g_dbus_connection_signal_subscribe (connection, + "org.freedesktop.DBus", + "org.freedesktop.DBus", + "NameAcquired", + "/org/freedesktop/DBus", + client->name, + G_DBUS_SIGNAL_FLAGS_NONE, + on_name_lost_or_acquired, + client, + NULL); + g_object_unref (connection); + } } client_unref (client); @@ -423,6 +452,15 @@ connection_get_cb (GObject *source_object, { Client *client = user_data; + /* must not do anything if already cancelled */ + G_LOCK (lock); + if (client->cancelled) + { + G_UNLOCK (lock); + goto out; + } + G_UNLOCK (lock); + client->connection = g_bus_get_finish (res, NULL); if (client->connection == NULL) { diff --git a/gio/tests/gdbus-names.c b/gio/tests/gdbus-names.c index 80e41a5..4af7ecb 100644 --- a/gio/tests/gdbus-names.c +++ b/gio/tests/gdbus-names.c @@ -213,6 +213,31 @@ test_bus_own_name (void) g_assert (!name_has_owner_reply); g_variant_unref (result); + /* Now try owning the name and then immediately decide to unown the name */ + g_assert_cmpint (data.num_bus_acquired, ==, 1); + g_assert_cmpint (data.num_acquired, ==, 1); + g_assert_cmpint (data.num_lost, ==, 0); + g_assert_cmpint (data.num_free_func, ==, 2); + id = g_bus_own_name (G_BUS_TYPE_SESSION, + name, + G_BUS_NAME_OWNER_FLAGS_NONE, + bus_acquired_handler, + name_acquired_handler, + name_lost_handler, + &data, + (GDestroyNotify) own_name_data_free_func); + g_assert_cmpint (data.num_bus_acquired, ==, 1); + g_assert_cmpint (data.num_acquired, ==, 1); + g_assert_cmpint (data.num_lost, ==, 0); + g_assert_cmpint (data.num_free_func, ==, 2); + g_bus_unown_name (id); + g_assert_cmpint (data.num_bus_acquired, ==, 1); + g_assert_cmpint (data.num_acquired, ==, 1); + g_assert_cmpint (data.num_lost, ==, 0); + g_assert_cmpint (data.num_free_func, ==, 2); + g_main_loop_run (loop); /* the GDestroyNotify is called in idle because the bus is acquired in idle */ + g_assert_cmpint (data.num_free_func, ==, 3); + /* * Own the name again. */ @@ -344,7 +369,7 @@ test_bus_own_name (void) g_bus_unown_name (id); g_assert_cmpint (data.num_bus_acquired, ==, 1); g_assert_cmpint (data.num_acquired, ==, 1); - g_assert_cmpint (data.num_free_func, ==, 3); + g_assert_cmpint (data.num_free_func, ==, 4); /* grab it again */ data.num_bus_acquired = 0; data.num_acquired = 0; @@ -443,7 +468,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_assert_cmpint (data.num_free_func, ==, 4); + g_assert_cmpint (data.num_free_func, ==, 5); _g_object_wait_for_single_ref (c); g_object_unref (c);