[Qemu-devel] [PATCH] monitor: move json init from OPEN event to init

[Qemu-devel] [PATCH] monitor: move json init from OPEN event to init

[Anthony Liguori]

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);
+
     QLIST_INSERT_HEAD(&mon_list, mon, entry);
     if (!default_mon || (flags & MONITOR_IS_DEFAULT))
         default_mon = mon;
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH] monitor: move json init from OPEN event to init

[Daniel P. Berrange]

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

Re: [Qemu-devel] [PATCH] monitor: move json init from OPEN event to init

[Max Filippov]

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

-- 
Thanks.
-- Max

Re: [Qemu-devel] [PATCH] monitor: move json init from OPEN event to init

[Max Filippov]

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.

-- 
Thanks.
-- Max