Hi


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&gt;;
发送时间:&nbsp;2024年4月10日(星期三) 晚上9:24
收件人:&nbsp;"ゞlym"<707242047@qq.com&gt;;
抄送:&nbsp;"qemu-devel"<qemu-devel@nongnu.org&gt;;"kraxel"<kraxel@redhat.com&gt;;
主题:&nbsp;Re: [PATCH RFC v1]display: fix heap use after free in cursor_put



Hi

On Wed, Apr 10, 2024 at 2:06 PM ゞlym <707242047@qq.com&gt; wrote:
&gt;
&gt;

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