Re: [Qemu-devel] [PATCH] monitor: Protect outbuf from concurrent access

[Gerd Hoffmann <kraxel@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1804 bytes --]

   Hi,

>> After some investigation, I found out that the problem is that different
>> SPICE threads are calling monitor functions (such as
>> monitor_protocol_event()) in parallel which causes concurrent access
>> to the monitor's internal buffer outbuf[].

[ adding spice-list to Cc, see qemu-devel for the rest of the thread ]

spice isn't supposed to do that.

/me just added a assert in channel_event() and saw it trigger in display 
channel disconnects.

#0  0x0000003ceba32a45 in raise () from /lib64/libc.so.6
#1  0x0000003ceba34225 in abort () from /lib64/libc.so.6
#2  0x0000003ceba2b9d5 in __assert_fail () from /lib64/libc.so.6
#3  0x0000000000503759 in channel_event (event=3, info=0x35e9340)
     at /home/kraxel/projects/qemu/ui/spice-core.c:223
#4  0x00007f9a77a9921b in reds_channel_event (s=0x35e92c0) at reds.c:400
#5  reds_stream_free (s=0x35e92c0) at reds.c:4981
#6  0x00007f9a77aac8b0 in red_disconnect_channel 
(channel=0x7f9a24069a80) at red_worker.c:8489
#7  0x00007f9a77ab53a8 in handle_dev_input (listener=0x7f9a3211ab20, 
events=<value optimized out>)
     at red_worker.c:10062
#8  0x00007f9a77ab436d in red_worker_main (arg=<value optimized out>) at 
red_worker.c:10304
#9  0x0000003cec2077e1 in start_thread () from /lib64/libpthread.so.0
#10 0x0000003cebae68ed in clone () from /lib64/libc.so.6

IMHO spice server should handle the display channel tear-down in the 
dispatcher instead of the worker thread.  Alon?

>> Anyways, this commit fixes the problem at hand.

Not really.  channel_event() itself isn't thread-safe too, it does 
unlocked list operations which can also blow up when called from 
different threads.

A patch like the attached (warning: untested) should do as quick&dirty 
fix for stable.  But IMO we really should fix spice instead.

cheers,
   Gerd


[-- Attachment #2: 0001-spice-workaround-a-spice-server-bug.patch --]
[-- Type: text/plain, Size: 2258 bytes --]

>From 7496e573ff6085d3c42d7e65b72c85fd2a7b4a78 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Fri, 2 Sep 2011 15:03:28 +0200
Subject: [PATCH] spice: workaround a spice server bug.

---
 ui/spice-core.c |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/ui/spice-core.c b/ui/spice-core.c
index dba11f0..c99cdc5 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -19,6 +19,7 @@
 #include <spice-experimental.h>
 
 #include <netdb.h>
+#include <pthread.h>
 
 #include "qemu-common.h"
 #include "qemu-spice.h"
@@ -44,6 +45,8 @@ static char *auth_passwd;
 static time_t auth_expires = TIME_MAX;
 int using_spice = 0;
 
+static pthread_t me;
+
 struct SpiceTimer {
     QEMUTimer *timer;
     QTAILQ_ENTRY(SpiceTimer) next;
@@ -216,6 +219,8 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
     };
     QDict *server, *client;
     QObject *data;
+    bool need_lock = !pthread_equal(me, pthread_self());
+    static int first = 1;
 
     client = qdict_new();
     add_addr_info(client, &info->paddr, info->plen);
@@ -223,6 +228,14 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
     server = qdict_new();
     add_addr_info(server, &info->laddr, info->llen);
 
+    if (need_lock) {
+        qemu_mutex_lock_iothread();
+        if (first) {
+            fprintf(stderr, "You are using a broken spice-server version\n");
+            first = 0;
+        }
+    }
+
     if (event == SPICE_CHANNEL_EVENT_INITIALIZED) {
         qdict_put(server, "auth", qstring_from_str(auth));
         add_channel_info(client, info);
@@ -236,6 +249,10 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
                               QOBJECT(client), QOBJECT(server));
     monitor_protocol_event(qevent[event], data);
     qobject_decref(data);
+
+    if (need_lock) {
+        qemu_mutex_unlock_iothread();
+    }
 }
 
 #else /* SPICE_INTERFACE_CORE_MINOR >= 3 */
@@ -482,7 +499,9 @@ void qemu_spice_init(void)
     spice_image_compression_t compression;
     spice_wan_compression_t wan_compr;
 
-    if (!opts) {
+    me = pthread_self();
+
+   if (!opts) {
         return;
     }
     port = qemu_opt_get_number(opts, "port", 0);
-- 
1.7.1