[v1] ui/dbus: fix cursor race, copy cursor data

[PATCH] ui/dbus: fix cursor race, copy cursor data

Posted by marcandre.lureau@redhat.com 4 weeks ago

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

Eleminates the undefined behavior where virtio-gpu's memcpy() overwrites
the buffer that an in-flight gvariant still references. Fixes the data
race where the GDBus worker thread calls cursor_unref() concurrently
with main thread.

Fixes: 142ca628a733 ("ui: add a D-Bus display backend")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/dbus-listener.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index 2e2f6ba4183a..aca802cb11cb 100644
--- a/ui/dbus-listener.c
+++ b/ui/dbus-listener.c
@@ -896,16 +896,18 @@ static void dbus_cursor_define(DisplayChangeListener *dcl,
 {
     DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
     GVariant *v_data = NULL;
+    size_t size = c->width * c->height * 4;
+    void *copy = g_memdup2(c->data, size);
 
     ddl_discard_cursor_messages(ddl);
 
     v_data = g_variant_new_from_data(
         G_VARIANT_TYPE("ay"),
-        c->data,
-        c->width * c->height * 4,
+        copy,
+        size,
         TRUE,
-        (GDestroyNotify)cursor_unref,
-        cursor_ref(c));
+        g_free,
+        copy);
 
     qemu_dbus_display1_listener_call_cursor_define(
         ddl->proxy,
-- 
2.55.0.543.g5ebe2ebe4ea8

Re: [PATCH] ui/dbus: fix cursor race, copy cursor data

Posted by Akihiko Odaki 3 weeks, 4 days ago

On 2026/08/29 23:14, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Eleminates the undefined behavior where virtio-gpu's memcpy() overwrites
> the buffer that an in-flight gvariant still references. Fixes the data
> race where the GDBus worker thread calls cursor_unref() concurrently
> with main thread.

s/Eleminates/Eliminates/

> 
> Fixes: 142ca628a733 ("ui: add a D-Bus display backend")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   ui/dbus-listener.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
> index 2e2f6ba4183a..aca802cb11cb 100644
> --- a/ui/dbus-listener.c
> +++ b/ui/dbus-listener.c
> @@ -896,16 +896,18 @@ static void dbus_cursor_define(DisplayChangeListener *dcl,
>   {
>       DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
>       GVariant *v_data = NULL;
> +    size_t size = c->width * c->height * 4;
> +    void *copy = g_memdup2(c->data, size);
>   
>       ddl_discard_cursor_messages(ddl);
>   
>       v_data = g_variant_new_from_data(
>           G_VARIANT_TYPE("ay"),
> -        c->data,
> -        c->width * c->height * 4,
> +        copy,
> +        size,
>           TRUE,
> -        (GDestroyNotify)cursor_unref,
> -        cursor_ref(c));
> +        g_free,
> +        copy);

This can use g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, ...), which 
internally performs the same copy and ownership setup.

Regards,
Akihiko Odaki

>   
>       qemu_dbus_display1_listener_call_cursor_define(
>           ddl->proxy,