During the test with logging, I found that there may be a conflict between the logic of updating the refcount in vnc_dpy_cursor_define() and QXL_CURSOR_SET action, same as dpy_cursor_define() after commit
385ac97f, and the atomic operation needs to be ensured;
The first thoughts are as follows??only lock cursor_unref/cursor_ref with ssd.lock??But it seems we can't get ssd.lock within dpy_cursor_define??so if we can't lock The top-level function qemu_spice_cursor_refresh_bh()??
--- a/hw/display/qxl-render.c
+++ b/hw/display/qxl-render.c
@@ -336,6 +336,7 @@ int qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext)
}
qemu_mutex_lock(&qxl->ssd.lock);
if (qxl->ssd.cursor) {
+ // other thread
cursor_unref(qxl->ssd.cursor);
}
qxl->ssd.cursor = c;
diff --git a/ui/console.c b/ui/console.c
index 43226c5c14..31dbd8fc6f 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -985,8 +985,10 @@ void dpy_cursor_define(QemuConsole *c, QEMUCursor *cursor)
DisplayState *s = c->ds;
DisplayChangeListener *dcl;
+ //lock, main thread
cursor_unref(con->cursor);
con->cursor = cursor_ref(cursor);
+ //unlock
if (!qemu_console_is_visible(c)) {
return;
}
------------------ ???????? ------------------
??????: "Marc-Andr?? Lureau" <marcandre.lureau@redhat.com>;
????????: 2024??4??10??(??????) ????9:24
??????: "?glym"<707242047@qq.com>;
????: "qemu-devel"<qemu-devel@nongnu.org>;"kraxel"<kraxel@redhat.com>;
????: Re: [PATCH RFC v1]display: fix heap use after free in cursor_put
Hi
On Wed, Apr 10, 2024 at 2:06?6?2PM ?glym <707242047@qq.com> wrote:
>
>
Please send the patch as inline:
https://www.qemu.org/docs/master/devel/submitting-a-patch.html#do-not-send-as-an-attachment
The patch is doing too much changes to the ssd.lock usage without
explaining in detail which race and how it solved it.
In particular, ui/spice-display.c usage seems safer before your
change, since it takes the lock on display_refresh and
display_mouse_define. It properly temporarily releases the lock before
calling the dpy_mouse_set() and dpy_cursor_define() as well.
To me, it looks like the only offender is qxl_spice_reset_cursor(),
which lacks locking before unrefing.
Could you confirm this hypothesis if you are able to reproduce the issue?
thanks