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

@@ -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,11 +345,25 @@ 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 */
+      if (connection != NULL)
+        {
           client->name_lost_subscription_id =
-        g_dbus_connection_signal_subscribe (client->connection,
+            g_dbus_connection_signal_subscribe (connection,
                                                 "org.freedesktop.DBus",
                                                 "org.freedesktop.DBus",
                                                 "NameLost",
@@ -347,7 +374,7 @@ request_name_cb (GObject      *source_object,
                                                 client,
                                                 NULL);
           client->name_acquired_subscription_id =
-        g_dbus_connection_signal_subscribe (client->connection,
+            g_dbus_connection_signal_subscribe (connection,
                                                 "org.freedesktop.DBus",
                                                 "org.freedesktop.DBus",
                                                 "NameAcquired",
@@ -357,6 +384,8 @@ request_name_cb (GObject      *source_object,
                                                 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)
     {

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);