[QEMU HCI-8.0 PATCH 2/7] ui/cursor: make the cursor refcount atomic #VSTOR-144000

["Denis V. Lunev" <den@openvz.org>]

From: Denis V. Lunev <den@openvz.org>

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: qemu-stable@nongnu.org
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 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 98feaa58bdd..a5db0b4e396 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -164,6 +164,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 6e23244fbe6..69d27d49a13 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