[glib: 1/2] gdbusnameowning: Subscribe to NameLost before calling RequestName

[Sebastian Dröge <sdroege src gnome org>]

commit ccbb2d8e8a034c9953621723bbc76b555f77c6b7
Author: Philip Withnall <withnall endlessm com>
Date:   Sun Feb 16 09:57:30 2020 +0000

    gdbusnameowning: Subscribe to NameLost before calling RequestName
    
    There was a slight race in name ownership: a gap between calling
    `RequestName` (or receiving its reply) and subscribing to `NameLost`. In
    that gap, another process could request and receive the name, and this
    one wouldn’t know about it.
    
    Fix that by subscribing to `NameAcquired` and `NameLost` before calling
    `RequestName`, and then unsubscribing again if the subscriptions turn
    out not to be necessary (if the process can’t own the requested name).
    
    Spotted and diagnosed by Miika Karanki.
    
    One of the tests needs an additional iteration of the main loop in order
    to free all the signal closures before it can complete its checks.
    
    Signed-off-by: Philip Withnall <withnall endlessm com>
    
    Fixes: #1517

 gio/gdbusnameowning.c   | 94 +++++++++++++++++++++++++++++--------------------
 gio/tests/gdbus-names.c |  1 +
 2 files changed, 56 insertions(+), 39 deletions(-)
---
diff --git a/gio/gdbusnameowning.c b/gio/gdbusnameowning.c
index 839a88cfb..ee7d8445f 100644
--- a/gio/gdbusnameowning.c
+++ b/gio/gdbusnameowning.c
@@ -310,7 +310,7 @@ request_name_cb (GObject      *source_object,
   Client *client = user_data;
   GVariant *result;
   guint32 request_name_reply;
-  gboolean subscribe;
+  gboolean unsubscribe;
 
   request_name_reply = 0;
   result = NULL;
@@ -325,22 +325,18 @@ request_name_cb (GObject      *source_object,
       g_variant_unref (result);
     }
 
-  subscribe = FALSE;
+  unsubscribe = FALSE;
 
   switch (request_name_reply)
     {
     case 1: /* DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER */
       /* We got the name - now listen for NameLost and NameAcquired */
       call_acquired_handler (client);
-      subscribe = TRUE;
-      client->needs_release = TRUE;
       break;
 
     case 2: /* DBUS_REQUEST_NAME_REPLY_IN_QUEUE */
       /* Waiting in line - listen for NameLost and NameAcquired */
       call_lost_handler (client);
-      subscribe = TRUE;
-      client->needs_release = TRUE;
       break;
 
     default:
@@ -349,53 +345,34 @@ request_name_cb (GObject      *source_object,
     case 4: /* DBUS_REQUEST_NAME_REPLY_ALREADY_OWNER */
       /* Some other part of the process is already owning the name */
       call_lost_handler (client);
+      unsubscribe = TRUE;
+      client->needs_release = FALSE;
       break;
     }
 
-
-  if (subscribe)
+  /* If we’re not the owner and not in the queue, there’s no point in continuing
+   * to listen to NameAcquired or NameLost. */
+  if (unsubscribe)
     {
       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
+      /* 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. 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 =
-            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_ref (client),
-                                                (GDestroyNotify) client_unref);
-          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_ref (client),
-                                                (GDestroyNotify) client_unref);
+          if (client->name_acquired_subscription_id > 0)
+            g_dbus_connection_signal_unsubscribe (client->connection, client->name_acquired_subscription_id);
+          if (client->name_lost_subscription_id > 0)
+            g_dbus_connection_signal_unsubscribe (client->connection, client->name_lost_subscription_id);
+          client->name_acquired_subscription_id = 0;
+          client->name_lost_subscription_id = 0;
+
           g_object_unref (connection);
         }
     }
@@ -439,7 +416,42 @@ has_connection (Client *client)
                                                              G_CALLBACK (on_connection_disconnected),
                                                              client);
 
+  /* 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().
+   *
+   * Subscribe to NameLost and NameAcquired before calling RequestName() to
+   * avoid the potential race of losing the name between receiving a reply to
+   * RequestName() and subscribing to NameLost. The #PreviousCall state will
+   * ensure that the user callbacks get called an appropriate number of times. */
+  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_ref (client),
+                                        (GDestroyNotify) client_unref);
+  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_ref (client),
+                                        (GDestroyNotify) client_unref);
+
   /* attempt to acquire the name */
+  client->needs_release = TRUE;
   g_dbus_connection_call (client->connection,
                           "org.freedesktop.DBus",  /* bus name */
                           "/org/freedesktop/DBus", /* object path */
@@ -944,6 +956,10 @@ g_bus_unown_name (guint owner_id)
                 {
                   g_warning ("Unexpected reply %d when releasing name %s", release_name_reply, client->name);
                 }
+              else
+                {
+                  client->needs_release = FALSE;
+                }
               g_variant_unref (result);
             }
         }
diff --git a/gio/tests/gdbus-names.c b/gio/tests/gdbus-names.c
index 94c777a30..20f429e58 100644
--- a/gio/tests/gdbus-names.c
+++ b/gio/tests/gdbus-names.c
@@ -297,6 +297,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, ==, 1);
   g_assert_cmpint (data2.num_acquired, ==, 0);
   g_assert_cmpint (data2.num_lost,     ==, 1);