gdbus: Fix race in name watching on connection teardown

If g_dbus_unwatch_name() is called from one thread at the same time as
the GDBusConnection is emitting ::disconnected in another thread, there
will be a race and the handler for ::disconnected may end up using
memory after it’s freed.

Fix this by serialising through the map_id_to_client, so that
on_connection_disconnected() atomically gets a strong reference to the
Client, or NULL.

https://bugzilla.gnome.org/show_bug.cgi?id=777307

1 files changed, 43 insertions, 5 deletions

diff --git a/gio/gdbusnamewatching.c b/gio/gdbusnamewatching.c
index a295587fa..9fb6dd1ca 100644
--- a/gio/gdbusnamewatching.c
+++ b/gio/gdbusnamewatching.c
@@ -249,13 +249,43 @@ call_vanished_handler (Client  *client,
 
 /* ---------------------------------------------------------------------------------------------------- */
 
+/* Return a reference to the #Client for @watcher_id, or %NULL if it’s been
+ * unwatched. This is safe to call from any thread. */
+static Client *
+dup_client (guint watcher_id)
+{
+  Client *client;
+
+  G_LOCK (lock);
+
+  g_assert (watcher_id != 0);
+  g_assert (map_id_to_client != NULL);
+
+  client = g_hash_table_lookup (map_id_to_client, GUINT_TO_POINTER (watcher_id));
+
+  if (client != NULL)
+    client_ref (client);
+
+  G_UNLOCK (lock);
+
+  return client;
+}
+
+/* Could be called from any thread, so it could be called after client_unref()
+ * has started finalising the #Client. Avoid that by looking up the #Client
+ * atomically. */
 static void
 on_connection_disconnected (GDBusConnection *connection,
                             gboolean         remote_peer_vanished,
                             GError          *error,
                             gpointer         user_data)
 {
-  Client *client = user_data;
+  guint watcher_id = GPOINTER_TO_UINT (user_data);
+  Client *client = NULL;
+
+  client = dup_client (watcher_id);
+  if (client == NULL)
+    return;
 
   if (client->name_owner_changed_subscription_id > 0)
     g_dbus_connection_signal_unsubscribe (client->connection, client->name_owner_changed_subscription_id);
@@ -267,10 +297,13 @@ on_connection_disconnected (GDBusConnection *connection,
   client->connection = NULL;
 
   call_vanished_handler (client, FALSE);
+
+  client_unref (client);
 }
 
 /* ---------------------------------------------------------------------------------------------------- */
 
+/* Will always be called from the thread which acquired client->main_context. */
 static void
 on_name_owner_changed (GDBusConnection *connection,
                        const gchar      *sender_name,
@@ -280,11 +313,16 @@ on_name_owner_changed (GDBusConnection *connection,
                        GVariant         *parameters,
                        gpointer          user_data)
 {
-  Client *client = user_data;
+  guint watcher_id = GPOINTER_TO_UINT (user_data);
+  Client *client = NULL;
   const gchar *name;
   const gchar *old_owner;
   const gchar *new_owner;
 
+  client = dup_client (watcher_id);
+  if (client == NULL)
+    return;
+
   if (!client->initialized)
     goto out;
 
@@ -319,7 +357,7 @@ on_name_owner_changed (GDBusConnection *connection,
     }
 
  out:
-  ;
+  client_unref (client);
 }
 
 /* ---------------------------------------------------------------------------------------------------- */
@@ -444,7 +482,7 @@ has_connection (Client *client)
   client->disconnected_signal_handler_id = g_signal_connect (client->connection,
                                                              "closed",
                                                              G_CALLBACK (on_connection_disconnected),
-                                                             client);
+                                                             GUINT_TO_POINTER (client->id));
 
   /* start listening to NameOwnerChanged messages immediately */
   client->name_owner_changed_subscription_id = g_dbus_connection_signal_subscribe (client->connection,
@@ -455,7 +493,7 @@ has_connection (Client *client)
                                                                                    client->name,
                                                                                    G_DBUS_SIGNAL_FLAGS_NONE,
                                                                                    on_name_owner_changed,
-                                                                                   client,
+                                                                                   GUINT_TO_POINTER (client->id),
                                                                                    NULL);
 
   if (client->flags & G_BUS_NAME_WATCHER_FLAGS_AUTO_START)