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

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

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
>