[glib] g_bus_own_name: fix race when unowning a name immediately after owning it

[David Zeuthen <davidz src gnome org>]

commit 1fc897352e2bd8c52f33517088213ee4b0024932
Author: David Zeuthen <davidz redhat com>
Date:   Thu Oct 27 10:30:58 2011 -0400

    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 <davidz redhat com>

 gio/gdbusnameowning.c   |   86 ++++++++++++++++++++++++++++++++++-------------
 gio/tests/gdbus-names.c |   29 +++++++++++++++-
 2 files changed, 89 insertions(+), 26 deletions(-)
---
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);