[PATCH] ui/dbus: associate add_client completion with its request

[PATCH] ui/dbus: associate add_client completion with its request

[GuoHan Zhao]

Commit 99997823bbbd ("ui/dbus: add p2p=on/off option")
introduced an asynchronous D-Bus client setup path, with the completion
handler reaching back into the global dbus_display state.

This makes the callback effectively operate on whatever request is
current when it runs, rather than the one that created it. A completion
from an older request can therefore clear a newer
add_client_cancellable or install its connection after a replacement
request has already been issued. It also relies on the DBusDisplay
instance remaining alive until completion.

Fix this by passing the DBusDisplay and GCancellable as callback data,
taking references while the async setup is in flight, and only acting
on completion if it still matches the current request. Also drop the
previous cancellable before creating a new request.

Fixes: 99997823bbbd ("ui/dbus: add p2p=on/off option")
Signed-off-by: GuoHan Zhao <zhaoguohan@kylinos.cn>
---
 ui/dbus.c | 44 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/ui/dbus.c b/ui/dbus.c
index 4f24215555a4..7c54b6a502d2 100644
--- a/ui/dbus.c
+++ b/ui/dbus.c
@@ -263,22 +263,52 @@ dbus_display_complete(UserCreatable *uc, Error **errp)
     }
 }
 
+typedef struct DBusDisplayAddClientData {
+    DBusDisplay *display;
+    GCancellable *cancellable;
+} DBusDisplayAddClientData;
+
+static void dbus_display_add_client_data_free(DBusDisplayAddClientData *data)
+{
+    if (data->display) {
+        object_unref(OBJECT(data->display));
+        data->display = NULL;
+    }
+    g_clear_object(&data->cancellable);
+    g_free(data);
+}
+
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(DBusDisplayAddClientData,
+                              dbus_display_add_client_data_free)
+
 static void
 dbus_display_add_client_ready(GObject *source_object,
                               GAsyncResult *res,
                               gpointer user_data)
 {
+    g_autoptr(DBusDisplayAddClientData) data = user_data;
+    DBusDisplay *display = data->display;
+    bool current = display->add_client_cancellable == data->cancellable;
     g_autoptr(GError) err = NULL;
     g_autoptr(GDBusConnection) conn = NULL;
 
-    g_clear_object(&dbus_display->add_client_cancellable);
+    if (current) {
+        g_clear_object(&display->add_client_cancellable);
+    }
 
     conn = g_dbus_connection_new_finish(res, &err);
     if (!conn) {
-        error_printf("Failed to accept D-Bus client: %s", err->message);
+        if (!g_error_matches(err, G_IO_ERROR, G_IO_ERROR_CANCELLED)) {
+            error_printf("Failed to accept D-Bus client: %s", err->message);
+        }
+        return;
     }
 
-    g_dbus_object_manager_server_set_connection(dbus_display->server, conn);
+    if (!current) {
+        return;
+    }
+
+    g_dbus_object_manager_server_set_connection(display->server, conn);
     g_dbus_connection_start_message_processing(conn);
 }
 
@@ -290,6 +320,7 @@ dbus_display_add_client(int csock, Error **errp)
     g_autoptr(GSocket) socket = NULL;
     g_autoptr(GSocketConnection) conn = NULL;
     g_autofree char *guid = g_dbus_generate_guid();
+    DBusDisplayAddClientData *data;
 
     if (!dbus_display) {
         error_setg(errp, "p2p connections not accepted in bus mode");
@@ -298,6 +329,7 @@ dbus_display_add_client(int csock, Error **errp)
 
     if (dbus_display->add_client_cancellable) {
         g_cancellable_cancel(dbus_display->add_client_cancellable);
+        g_clear_object(&dbus_display->add_client_cancellable);
     }
 
 #ifdef WIN32
@@ -318,6 +350,10 @@ dbus_display_add_client(int csock, Error **errp)
     conn = g_socket_connection_factory_create_connection(socket);
 
     dbus_display->add_client_cancellable = g_cancellable_new();
+    data = g_new0(DBusDisplayAddClientData, 1);
+    data->display = DBUS_DISPLAY(object_ref(OBJECT(dbus_display)));
+    data->cancellable = g_object_ref(dbus_display->add_client_cancellable);
+
     GDBusConnectionFlags flags =
         G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_SERVER |
         G_DBUS_CONNECTION_FLAGS_DELAY_MESSAGE_PROCESSING;
@@ -332,7 +368,7 @@ dbus_display_add_client(int csock, Error **errp)
                           NULL,
                           dbus_display->add_client_cancellable,
                           dbus_display_add_client_ready,
-                          NULL);
+                          data);
 
     return true;
 }
-- 
2.43.0

Re: [PATCH] ui/dbus: associate add_client completion with its request

[Marc-André Lureau]

Hi

On Thu, Mar 26, 2026 at 10:56 AM GuoHan Zhao <zhaoguohan@kylinos.cn> wrote:
>
> Commit 99997823bbbd ("ui/dbus: add p2p=on/off option")
> introduced an asynchronous D-Bus client setup path, with the completion
> handler reaching back into the global dbus_display state.
>
> This makes the callback effectively operate on whatever request is
> current when it runs, rather than the one that created it. A completion
> from an older request can therefore clear a newer
> add_client_cancellable or install its connection after a replacement
> request has already been issued. It also relies on the DBusDisplay
> instance remaining alive until completion.
>
> Fix this by passing the DBusDisplay and GCancellable as callback data,
> taking references while the async setup is in flight, and only acting
> on completion if it still matches the current request. Also drop the
> previous cancellable before creating a new request.
>
> Fixes: 99997823bbbd ("ui/dbus: add p2p=on/off option")
> Signed-off-by: GuoHan Zhao <zhaoguohan@kylinos.cn>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

thanks!

> ---
>  ui/dbus.c | 44 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 40 insertions(+), 4 deletions(-)
>
> diff --git a/ui/dbus.c b/ui/dbus.c
> index 4f24215555a4..7c54b6a502d2 100644
> --- a/ui/dbus.c
> +++ b/ui/dbus.c
> @@ -263,22 +263,52 @@ dbus_display_complete(UserCreatable *uc, Error **errp)
>      }
>  }
>
> +typedef struct DBusDisplayAddClientData {
> +    DBusDisplay *display;
> +    GCancellable *cancellable;
> +} DBusDisplayAddClientData;
> +
> +static void dbus_display_add_client_data_free(DBusDisplayAddClientData *data)
> +{
> +    if (data->display) {
> +        object_unref(OBJECT(data->display));
> +        data->display = NULL;
> +    }
> +    g_clear_object(&data->cancellable);
> +    g_free(data);
> +}
> +
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(DBusDisplayAddClientData,
> +                              dbus_display_add_client_data_free)
> +
>  static void
>  dbus_display_add_client_ready(GObject *source_object,
>                                GAsyncResult *res,
>                                gpointer user_data)
>  {
> +    g_autoptr(DBusDisplayAddClientData) data = user_data;
> +    DBusDisplay *display = data->display;
> +    bool current = display->add_client_cancellable == data->cancellable;
>      g_autoptr(GError) err = NULL;
>      g_autoptr(GDBusConnection) conn = NULL;
>
> -    g_clear_object(&dbus_display->add_client_cancellable);
> +    if (current) {
> +        g_clear_object(&display->add_client_cancellable);
> +    }
>
>      conn = g_dbus_connection_new_finish(res, &err);
>      if (!conn) {
> -        error_printf("Failed to accept D-Bus client: %s", err->message);
> +        if (!g_error_matches(err, G_IO_ERROR, G_IO_ERROR_CANCELLED)) {
> +            error_printf("Failed to accept D-Bus client: %s", err->message);
> +        }
> +        return;
>      }
>
> -    g_dbus_object_manager_server_set_connection(dbus_display->server, conn);
> +    if (!current) {
> +        return;
> +    }
> +
> +    g_dbus_object_manager_server_set_connection(display->server, conn);
>      g_dbus_connection_start_message_processing(conn);
>  }
>
> @@ -290,6 +320,7 @@ dbus_display_add_client(int csock, Error **errp)
>      g_autoptr(GSocket) socket = NULL;
>      g_autoptr(GSocketConnection) conn = NULL;
>      g_autofree char *guid = g_dbus_generate_guid();
> +    DBusDisplayAddClientData *data;
>
>      if (!dbus_display) {
>          error_setg(errp, "p2p connections not accepted in bus mode");
> @@ -298,6 +329,7 @@ dbus_display_add_client(int csock, Error **errp)
>
>      if (dbus_display->add_client_cancellable) {
>          g_cancellable_cancel(dbus_display->add_client_cancellable);
> +        g_clear_object(&dbus_display->add_client_cancellable);
>      }
>
>  #ifdef WIN32
> @@ -318,6 +350,10 @@ dbus_display_add_client(int csock, Error **errp)
>      conn = g_socket_connection_factory_create_connection(socket);
>
>      dbus_display->add_client_cancellable = g_cancellable_new();
> +    data = g_new0(DBusDisplayAddClientData, 1);
> +    data->display = DBUS_DISPLAY(object_ref(OBJECT(dbus_display)));
> +    data->cancellable = g_object_ref(dbus_display->add_client_cancellable);
> +
>      GDBusConnectionFlags flags =
>          G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_SERVER |
>          G_DBUS_CONNECTION_FLAGS_DELAY_MESSAGE_PROCESSING;
> @@ -332,7 +368,7 @@ dbus_display_add_client(int csock, Error **errp)
>                            NULL,
>                            dbus_display->add_client_cancellable,
>                            dbus_display_add_client_ready,
> -                          NULL);
> +                          data);
>
>      return true;
>  }
> --
> 2.43.0
>