Patchwork monitor: move json init from OPEN event to init

login
register
mail settings
Submitter Anthony Liguori
Date Aug. 23, 2012, 1:22 p.m.
Message ID <1345728155-11667-1-git-send-email-aliguori@us.ibm.com>
Download mbox | patch
Permalink /patch/179650/
State New
Headers show

Comments

Anthony Liguori - Aug. 23, 2012, 1:22 p.m.
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(-)
Daniel P. Berrange - Aug. 23, 2012, 1:42 p.m.
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
Max Filippov - Aug. 23, 2012, 10:06 p.m.
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
>
>
Max Filippov - Aug. 23, 2012, 10:18 p.m.
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.

Patch

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;