Re: [Qemu-devel] Race condition in char device setup causing SEGV

[Daniel P. Berrange]

On Thu, Aug 23, 2012 at 12:01:05PM +0100, Daniel P. Berrange wrote:
> When testing with the new "-M none" arg, I've noticed that ~70%
> of the time libvirt starts QEMU will result in a SEGV from QEMU
> with the following stack trace:
> 
> (gdb) bt
> #0  0x0000000000000000 in ?? ()
> #1  0x000055555567a37f in json_lexer_feed_char (lexer=0x55555658fb20, ch=123 
> '{', flush=false) at json-lexer.c:324
> #2  0x000055555567a4aa in json_lexer_feed (lexer=0x55555658fb20, 
> buffer=0x7fffffffe7b7 "{", size=1) at json-lexer.c:356
> #3  0x000055555567c708 in json_message_parser_feed (parser=0x55555658fb18, 
> buffer=0x7fffffffe7b7 "{", size=1) at json-streamer.c:110
> #4  0x0000555555882861 in monitor_control_read (opaque=0x55555658f6a0, 
> buf=0x7fffffffe7b7 "{", size=1) at /home/berrange/src/virt/qemu/monitor.c:4768
> #5  0x000055555579b051 in qemu_chr_be_write (s=0x55555658dc10, 
> buf=0x7fffffffe7b7 "{", len=1) at qemu-char.c:164
> #6  0x000055555579c9c8 in stdio_read (opaque=0x55555658dc10) at 
> qemu-char.c:720
> #7  0x000055555567941f in qemu_iohandler_poll (readfds=0x5555560f17c0, 
> writefds=0x5555560f1840, xfds=0x5555560f18c0, ret=2) at iohandler.c:122
> #8  0x000055555577166a in main_loop_wait (nonblocking=0) at main-loop.c:497
> #9  0x000055555576956b in main_loop () at 
> /home/berrange/src/virt/qemu/vl.c:1643
> #10 0x0000555555770239 in main (argc=10, argv=0x7fffffffeca8, 
> envp=0x7fffffffed00) at /home/berrange/src/virt/qemu/vl.c:3755
> 
> 
> Stack frame #1 there is doing this:
> 
>   lexer->emit(lexer, lexer->token, JSON_ERROR, lexer->x, lexer->y);
> 
> GDB confirms that the 'emit' field has not yet been initialized.
> 
> In the case of QMP, this is initialized by the following sequence:
> 
>  - main
>  - chardev_init_func
>  - qemu_chr_generic_open
> 
>  ...async from event loop...
> 
>  - main_loop
>  - qemu_chr_generic_open_bh
>  - monitor_control_event
>  - json_message_parser_init
>  - json_lexer_init
> 
> 
> The problem arises if you try to feed data to QEMU before the bottom
> half has run. There is a race where qemu_chr_be_write can be called
> to process input, before the qemu_chr_generic_open_bh has been invoked.
> 
> This can actually be quite easily demonstrated (at least on my system):
> 
>  # echo "{" | qemu-system-x86_64 -nodefaults -nographic -M none -qmp stdio
>  Segmentation fault
> 
> If you remove the '-M none' call, you won't hit this race condition 99%
> of the time, but I have occassionally been able to see it.
> 
> It isn't clear to me what to change to solve this race condition. Probably
> though, the I/O handlers for a char device should be registered until the
> open bottom half has completed.

The following patch shows one approach to fix it. I've only addressed
the 'stdio' char backend - obviously a complete patch would try todo
all backends where needed.

diff --git a/qemu-char.c b/qemu-char.c
index 398baf1..85034de 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -118,6 +118,9 @@ void qemu_chr_be_event(CharDriverState *s, int event)
             break;
     }
 
+    if (s->chr_event_priv)
+       s->chr_event_priv(s, event);
+
     if (!s->chr_event)
         return;
     s->chr_event(s->handler_opaque, event);
@@ -765,6 +768,22 @@ static void qemu_chr_close_stdio(struct CharDriverState 
*chr)
     fd_chr_close(chr);
 }
 
+
+static void qemu_chr_event_stdio(void *opaque, int event)
+{
+    CharDriverState *chr = opaque;
+
+    switch (event) {
+    case CHR_EVENT_OPENED:
+       qemu_set_fd_handler2(0, stdio_read_poll, stdio_read, NULL, chr);
+       break;
+    case CHR_EVENT_CLOSED:
+       qemu_set_fd_handler2(0, NULL, NULL, NULL, NULL);
+       break;
+    }
+}
+
+
 static CharDriverState *qemu_chr_open_stdio(QemuOpts *opts)
 {
     CharDriverState *chr;
@@ -782,7 +801,7 @@ static CharDriverState *qemu_chr_open_stdio(QemuOpts *opts)
     chr = qemu_chr_open_fd(0, 1);
     chr->chr_close = qemu_chr_close_stdio;
     chr->chr_set_echo = qemu_chr_set_echo_stdio;
-    qemu_set_fd_handler2(0, stdio_read_poll, stdio_read, NULL, chr);
+    chr->chr_event_priv = qemu_chr_event_stdio;
     stdio_nb_clients++;
     stdio_allow_signal = qemu_opt_get_bool(opts, "signal",
                                            display_type != DT_NOGRAPHIC);
diff --git a/qemu-char.h b/qemu-char.h
index 486644b..e377ae7 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -59,6 +59,7 @@ struct CharDriverState {
     int (*chr_ioctl)(struct CharDriverState *s, int cmd, void *arg);
     int (*get_msgfd)(struct CharDriverState *s);
     int (*chr_add_client)(struct CharDriverState *chr, int fd);
+    IOEventHandler *chr_event_priv;
     IOEventHandler *chr_event;
     IOCanReadHandler *chr_can_read;
     IOReadHandler *chr_read;


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|