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 :|
