From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:51531) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QzTxg-0002bW-PD for qemu-devel@nongnu.org; Fri, 02 Sep 2011 09:39:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QzTxf-0004AU-Jo for qemu-devel@nongnu.org; Fri, 02 Sep 2011 09:39:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:24570) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QzTxf-0004AL-BV for qemu-devel@nongnu.org; Fri, 02 Sep 2011 09:39:11 -0400 Message-ID: <4E60DC77.5020300@redhat.com> Date: Fri, 02 Sep 2011 15:39:03 +0200 From: Gerd Hoffmann MIME-Version: 1.0 References: <20110901163545.71ba1515@doriath> <4E6032AB.8080804@codemonkey.ws> In-Reply-To: <4E6032AB.8080804@codemonkey.ws> Content-Type: multipart/mixed; boundary="------------050007000106030403050000" Subject: Re: [Qemu-devel] [PATCH] monitor: Protect outbuf from concurrent access List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Marian Krcmarik , Alon Levy , qemu-devel , spice-devel , Luiz Capitulino This is a multi-part message in MIME format. --------------050007000106030403050000 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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=) at red_worker.c:10062 #8 0x00007f9a77ab436d in red_worker_main (arg=) 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 --------------050007000106030403050000 Content-Type: text/plain; name="0001-spice-workaround-a-spice-server-bug.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0001-spice-workaround-a-spice-server-bug.patch" >>From 7496e573ff6085d3c42d7e65b72c85fd2a7b4a78 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann 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 #include +#include #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 --------------050007000106030403050000--