| Message ID | 1345728155-11667-1-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Aug 23, 2012 at 08:22:35AM -0500, Anthony Liguori wrote: > At some point in the past, the OPEN event was changed to be issued from a > bottom half. This creates a small window whereas a data callback registered in > init may be invoked before the OPEN event has been issued. > > This is reproducible with: > > echo "{'execute': 'qmp_capabilities'}" | qemu-system-x86_64 -M none -qmp stdio > > We can fix this for the monitor by moving the parser initialization to init. > > The remaining state that is set in OPEN appears harmless. > > Reported-by: Daniel Berrange <berrange@redhat.com> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> Signed-off-by: Daniel P. Berrange <berrange@redhat.com> Works in my testing. I have also fixed libvirt so that it does not try to send commands to QMP, until QMP has sent out its initial greeting, thus avoiding the flaw too. Daniel
On Thu, Aug 23, 2012 at 5:22 PM, Anthony Liguori <aliguori@us.ibm.com> wrote: > At some point in the past, the OPEN event was changed to be issued from a > bottom half. This creates a small window whereas a data callback registered in > init may be invoked before the OPEN event has been issued. > > This is reproducible with: > > echo "{'execute': 'qmp_capabilities'}" | qemu-system-x86_64 -M none -qmp stdio > > We can fix this for the monitor by moving the parser initialization to init. > > The remaining state that is set in OPEN appears harmless. > > Reported-by: Daniel Berrange <berrange@redhat.com> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > --- > monitor.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/monitor.c b/monitor.c > index 480f583..b188582 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -4832,7 +4832,6 @@ static void monitor_control_event(void *opaque, int event) > switch (event) { > case CHR_EVENT_OPENED: > mon->mc->command_mode = 0; > - json_message_parser_init(&mon->mc->parser, handle_qmp_command); > data = get_qmp_greeting(); > monitor_json_emitter(mon, data); > qobject_decref(data); > @@ -4840,6 +4839,7 @@ static void monitor_control_event(void *opaque, int event) > break; > case CHR_EVENT_CLOSED: > json_message_parser_destroy(&mon->mc->parser); > + json_message_parser_init(&mon->mc->parser, handle_qmp_command); > mon_refcount--; > monitor_fdsets_cleanup(); > break; > @@ -4951,6 +4951,8 @@ void monitor_init(CharDriverState *chr, int flags) > monitor_event, mon); > } > > + json_message_parser_init(&mon->mc->parser, handle_qmp_command); > + This hunk causes SIGSEGV on qemu-system-xtensa with the following trace: Program received signal SIGSEGV, Segmentation fault. json_message_parser_init (parser=0x8, func=0x5555556b4db0 <handle_qmp_command>) at qemu/json-streamer.c:98 98 parser->emit = func; (gdb) bt #0 json_message_parser_init (parser=0x8, func=0x5555556b4db0 <handle_qmp_command>) at qemu/json-streamer.c:98 #1 0x00005555556ba5c7 in monitor_init (chr=0x555556228fe0, flags=2) at qemu/monitor.c:4954 #2 0x000055555564f83d in qemu_chr_new (label=<optimized out>, filename=<optimized out>, init=0) at qemu/qemu-char.c:2828 #3 0x0000555555626525 in serial_parse (devname=0x5555556f4152 "mon:stdio") at qemu/vl.c:2068 #4 serial_parse (devname=<optimized out>) at qemu/vl.c:2056 #5 0x0000555555625009 in foreach_device_config (type=2, func=0x5555556264b0 <serial_parse>) at qemu/vl.c:2048 #6 0x00005555555973f5 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at qemu/vl.c:3588 The command line is the following: qemu-system-xtensa -M sim -cpu dc232b -nographic -semihosting -kernel ./test_b.tst > QLIST_INSERT_HEAD(&mon_list, mon, entry); > if (!default_mon || (flags & MONITOR_IS_DEFAULT)) > default_mon = mon; > -- > 1.7.5.4 > >
On Fri, Aug 24, 2012 at 2:06 AM, Max Filippov <jcmvbkbc@gmail.com> wrote: > On Thu, Aug 23, 2012 at 5:22 PM, Anthony Liguori <aliguori@us.ibm.com> wrote: >> At some point in the past, the OPEN event was changed to be issued from a >> bottom half. This creates a small window whereas a data callback registered in >> init may be invoked before the OPEN event has been issued. >> >> This is reproducible with: >> >> echo "{'execute': 'qmp_capabilities'}" | qemu-system-x86_64 -M none -qmp stdio >> >> We can fix this for the monitor by moving the parser initialization to init. >> >> The remaining state that is set in OPEN appears harmless. >> >> Reported-by: Daniel Berrange <berrange@redhat.com> >> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> >> --- >> monitor.c | 4 +++- >> 1 files changed, 3 insertions(+), 1 deletions(-) >> >> diff --git a/monitor.c b/monitor.c >> index 480f583..b188582 100644 >> --- a/monitor.c >> +++ b/monitor.c >> @@ -4832,7 +4832,6 @@ static void monitor_control_event(void *opaque, int event) >> switch (event) { >> case CHR_EVENT_OPENED: >> mon->mc->command_mode = 0; >> - json_message_parser_init(&mon->mc->parser, handle_qmp_command); >> data = get_qmp_greeting(); >> monitor_json_emitter(mon, data); >> qobject_decref(data); >> @@ -4840,6 +4839,7 @@ static void monitor_control_event(void *opaque, int event) >> break; >> case CHR_EVENT_CLOSED: >> json_message_parser_destroy(&mon->mc->parser); >> + json_message_parser_init(&mon->mc->parser, handle_qmp_command); >> mon_refcount--; >> monitor_fdsets_cleanup(); >> break; >> @@ -4951,6 +4951,8 @@ void monitor_init(CharDriverState *chr, int flags) >> monitor_event, mon); >> } >> >> + json_message_parser_init(&mon->mc->parser, handle_qmp_command); >> + > > This hunk causes SIGSEGV on qemu-system-xtensa with the following trace: I see that '[PATCH] monitor: don't try to initialize json parser when monitor is HMP' is meant to fix it.
diff --git a/monitor.c b/monitor.c index 480f583..b188582 100644 --- a/monitor.c +++ b/monitor.c @@ -4832,7 +4832,6 @@ static void monitor_control_event(void *opaque, int event) switch (event) { case CHR_EVENT_OPENED: mon->mc->command_mode = 0; - json_message_parser_init(&mon->mc->parser, handle_qmp_command); data = get_qmp_greeting(); monitor_json_emitter(mon, data); qobject_decref(data); @@ -4840,6 +4839,7 @@ static void monitor_control_event(void *opaque, int event) break; case CHR_EVENT_CLOSED: json_message_parser_destroy(&mon->mc->parser); + json_message_parser_init(&mon->mc->parser, handle_qmp_command); mon_refcount--; monitor_fdsets_cleanup(); break; @@ -4951,6 +4951,8 @@ void monitor_init(CharDriverState *chr, int flags) monitor_event, mon); } + json_message_parser_init(&mon->mc->parser, handle_qmp_command); + QLIST_INSERT_HEAD(&mon_list, mon, entry); if (!default_mon || (flags & MONITOR_IS_DEFAULT)) default_mon = mon;
At some point in the past, the OPEN event was changed to be issued from a bottom half. This creates a small window whereas a data callback registered in init may be invoked before the OPEN event has been issued. This is reproducible with: echo "{'execute': 'qmp_capabilities'}" | qemu-system-x86_64 -M none -qmp stdio We can fix this for the monitor by moving the parser initialization to init. The remaining state that is set in OPEN appears harmless. Reported-by: Daniel Berrange <berrange@redhat.com> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> --- monitor.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)