On Thu, Sep 3, 2026 at 11:27 PM Denis V. Lunev <[email protected]> wrote: > > From: Denis V. Lunev <[email protected]> > > A QEMUCursor outlives the call that publishes it and is shared between > threads, but its refcount was a plain int with no single lock covering > every user. qemu_console_set_cursor() takes and drops references from > the main loop under the BQL alone, hw/display/qxl-render.c does so from > the SPICE display worker thread, and ui/spice-display.c does so under > SimpleSpiceDisplay::lock. ui/cocoa.m and ui/dbus-listener.c add two more > threads. > > The pair that collides is qemu_spice_cursor_refresh_bh(), which drops > ssd->lock before calling qemu_console_set_cursor(), and the worker > refcounting the same cursor under that lock. A lost increment frees the > cursor while the console still points at it, so the console's next unref > decrements memory the allocator has already handed out again. Locking > ssd.cursor is not enough on its own: with that done, this is the race > that remains. > > Assert on the value the decrement observed while here. Dropping a > reference that was never taken used to be silent, because the decrement > lands in the allocator metadata of the freed chunk: nothing is logged, > the object is not freed twice, and the process runs on until some later > allocation walks the damaged free list and faults, arbitrarily far from > the code that caused it. > > Fixes: 0b2824e5e48a ("spice: use bottom half instead of refresh timer for > cursor updates") > Cc: [email protected] > Cc: Marc-André Lureau <[email protected]> > Signed-off-by: Denis V. Lunev <[email protected]>
I suspected that, never had the time or motivation to find the arguments: Reviewed-by: Marc-André Lureau <[email protected]> thanks > --- > include/ui/console.h | 9 +++++++++ > ui/cursor.c | 17 +++++++++++------ > 2 files changed, 20 insertions(+), 6 deletions(-) > > diff --git a/include/ui/console.h b/include/ui/console.h > index 29bf722888..3634956949 100644 > --- a/include/ui/console.h > +++ b/include/ui/console.h > @@ -126,6 +126,15 @@ typedef struct QEMUCursor { > } QEMUCursor; > > QEMUCursor *cursor_alloc(uint16_t width, uint16_t height); > + > +/* > + * A cursor may be shared between the main loop, a vCPU thread and a > + * display backend's own thread, so the refcount is atomic and these two > + * may be called from any of them. The object itself is not otherwise > + * thread-safe: take a reference before publishing the pointer anywhere > + * another thread can reach it, and never dereference a cursor you do > + * not hold a reference to. > + */ > QEMUCursor *cursor_ref(QEMUCursor *c); > void cursor_unref(QEMUCursor *c); > QEMUCursor *cursor_builtin_hidden(void); > diff --git a/ui/cursor.c b/ui/cursor.c > index 6e23244fbe..69d27d49a1 100644 > --- a/ui/cursor.c > +++ b/ui/cursor.c > @@ -1,4 +1,5 @@ > #include "qemu/osdep.h" > +#include "qemu/atomic.h" > #include "ui/console.h" > > #include "cursor_hidden.xpm" > @@ -103,24 +104,28 @@ QEMUCursor *cursor_alloc(uint16_t width, uint16_t > height) > c = g_malloc0(sizeof(QEMUCursor) + datasize); > c->width = width; > c->height = height; > - c->refcount = 1; > + qatomic_set(&c->refcount, 1); > return c; > } > > QEMUCursor *cursor_ref(QEMUCursor *c) > { > - c->refcount++; > + qatomic_inc(&c->refcount); > return c; > } > > void cursor_unref(QEMUCursor *c) > { > + int refcount; > + > if (c == NULL) > return; > - c->refcount--; > - if (c->refcount) > - return; > - g_free(c); > + > + refcount = qatomic_fetch_dec(&c->refcount); > + assert(refcount > 0); > + if (refcount == 1) { > + g_free(c); > + } > } > > int cursor_get_mono_bpl(QEMUCursor *c) > -- > 2.53.0 >
