[Qemu-devel] [PULL v2 18/32] qmp: Don't let JSON errors jump the queue

Markus Armbruster posted 32 patches 7 years, 4 months ago
[Qemu-devel] [PULL v2 18/32] qmp: Don't let JSON errors jump the queue
Posted by Markus Armbruster 7 years, 4 months ago
handle_qmp_command() reports JSON syntax errors right away.  This is
wrong when OOB is enabled, because the errors can "jump the queue"
then.

The previous commit fixed the same bug for semantic errors, by
delaying the checking until dispatch.  We can't delay the checking, so
delay the reporting.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20180703085358.13941-19-armbru@redhat.com>
---
 monitor.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/monitor.c b/monitor.c
index be2a856d1c..ea5421399a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -254,8 +254,12 @@ struct QMPRequest {
     Monitor *mon;
     /* "id" field of the request */
     QObject *id;
-    /* Request object to be handled */
+    /*
+     * Request object to be handled or Error to be reported
+     * (exactly one of them is non-null)
+     */
     QObject *req;
+    Error *err;
     /*
      * Whether we need to resume the monitor afterward.  This flag is
      * used to emulate the old QMP server behavior that the current
@@ -360,6 +364,7 @@ static void qmp_request_free(QMPRequest *req)
 {
     qobject_unref(req->id);
     qobject_unref(req->req);
+    error_free(req->err);
     g_free(req);
 }
 
@@ -4199,8 +4204,14 @@ static void monitor_qmp_bh_dispatcher(void *data)
         return;
     }
 
-    trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
-    monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
+    if (req_obj->req) {
+        trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
+        monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
+    } else {
+        assert(req_obj->err);
+        monitor_qmp_respond(req_obj->mon, NULL, req_obj->err, NULL);
+    }
+
     if (req_obj->need_resume) {
         /* Pairs with the monitor_suspend() in handle_qmp_command() */
         monitor_resume(req_obj->mon);
@@ -4227,11 +4238,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
         /* json_parser_parse_err() sucks: can fail without setting @err */
         error_setg(&err, QERR_JSON_PARSING);
     }
-    if (err) {
-        assert(!req);
-        monitor_qmp_respond(mon, NULL, err, NULL);
-        return;
-    }
 
     qdict = qobject_to(QDict, req);
     if (qdict) {
@@ -4257,6 +4263,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
     req_obj->mon = mon;
     req_obj->id = id;
     req_obj->req = req;
+    req_obj->err = err;
     req_obj->need_resume = false;
 
     /* Protect qmp_requests and fetching its length. */
-- 
2.17.1


Re: [Qemu-devel] [PULL v2 18/32] qmp: Don't let JSON errors jump the queue
Posted by Kevin Wolf 7 years, 3 months ago
Am 03.07.2018 um 23:35 hat Markus Armbruster geschrieben:
> handle_qmp_command() reports JSON syntax errors right away.  This is
> wrong when OOB is enabled, because the errors can "jump the queue"
> then.
> 
> The previous commit fixed the same bug for semantic errors, by
> delaying the checking until dispatch.  We can't delay the checking, so
> delay the reporting.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Message-Id: <20180703085358.13941-19-armbru@redhat.com>

I'm observing a qemu crash in qemu-iotests 153 (which does however not
seem to make the test case fail). git bisect points me to this patch.

I'm getting output like this:

*** Error in `/home/kwolf/source/qemu/tests/qemu-iotests/qemu': free(): invalid pointer: 0x0000555f7870f7e0 ***
======= Backtrace: =========
/lib64/libc.so.6(+0x7cbac)[0x7fa9b29a2bac]
/lib64/libc.so.6(+0x87a59)[0x7fa9b29ada59]
/lib64/libc.so.6(cfree+0x16e)[0x7fa9b29b33be]
/lib64/libglib-2.0.so.0(g_free+0xe)[0x7fa9ce462b4e]
/home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6eb9dc)[0x555f76f489dc]
/home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x30ae4b)[0x555f76b67e4b]
/home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x311558)[0x555f76b6e558]
/home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e2d4e)[0x555f76f3fd4e]
/home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e5fa0)[0x555f76f42fa0]
/home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e2c2e)[0x555f76f3fc2e]
/lib64/libglib-2.0.so.0(g_main_context_dispatch+0x157)[0x7fa9ce45d257]
/home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e526e)[0x555f76f4226e]
/home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x42349e)[0x555f76c8049e]
/home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x2c27ef)[0x555f76b1f7ef]
/lib64/libc.so.6(__libc_start_main+0xea)[0x7fa9b294688a]
/home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x2c5b8a)[0x555f76b22b8a]

Interestingly, this doesn't want to produce a core dump for me, so no
backtrace with usable function names here. But I assume that you can
easily reproduce this yourself.

Kevin

Re: [Qemu-devel] [PULL v2 18/32] qmp: Don't let JSON errors jump the queue
Posted by Marc-André Lureau 7 years, 3 months ago
Hi

On Tue, Jul 10, 2018 at 3:20 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 03.07.2018 um 23:35 hat Markus Armbruster geschrieben:
>> handle_qmp_command() reports JSON syntax errors right away.  This is
>> wrong when OOB is enabled, because the errors can "jump the queue"
>> then.
>>
>> The previous commit fixed the same bug for semantic errors, by
>> delaying the checking until dispatch.  We can't delay the checking, so
>> delay the reporting.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Message-Id: <20180703085358.13941-19-armbru@redhat.com>
>
> I'm observing a qemu crash in qemu-iotests 153 (which does however not
> seem to make the test case fail). git bisect points me to this patch.
>
> I'm getting output like this:
>
> *** Error in `/home/kwolf/source/qemu/tests/qemu-iotests/qemu': free(): invalid pointer: 0x0000555f7870f7e0 ***
> ======= Backtrace: =========
> /lib64/libc.so.6(+0x7cbac)[0x7fa9b29a2bac]
> /lib64/libc.so.6(+0x87a59)[0x7fa9b29ada59]
> /lib64/libc.so.6(cfree+0x16e)[0x7fa9b29b33be]
> /lib64/libglib-2.0.so.0(g_free+0xe)[0x7fa9ce462b4e]
> /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6eb9dc)[0x555f76f489dc]
> /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x30ae4b)[0x555f76b67e4b]
> /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x311558)[0x555f76b6e558]
> /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e2d4e)[0x555f76f3fd4e]
> /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e5fa0)[0x555f76f42fa0]
> /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e2c2e)[0x555f76f3fc2e]
> /lib64/libglib-2.0.so.0(g_main_context_dispatch+0x157)[0x7fa9ce45d257]
> /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e526e)[0x555f76f4226e]
> /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x42349e)[0x555f76c8049e]
> /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x2c27ef)[0x555f76b1f7ef]
> /lib64/libc.so.6(__libc_start_main+0xea)[0x7fa9b294688a]
> /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x2c5b8a)[0x555f76b22b8a]
>
> Interestingly, this doesn't want to produce a core dump for me, so no
> backtrace with usable function names here. But I assume that you can
> easily reproduce this yourself.
>

Looks like the double-free regression, you could try: "[PATCH]
monitor: fix double-free of request error"

thanks



-- 
Marc-André Lureau

Re: [Qemu-devel] [PULL v2 18/32] qmp: Don't let JSON errors jump the queue
Posted by Kevin Wolf 7 years, 3 months ago
Am 10.07.2018 um 16:02 hat Marc-André Lureau geschrieben:
> Hi
> 
> On Tue, Jul 10, 2018 at 3:20 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> > Am 03.07.2018 um 23:35 hat Markus Armbruster geschrieben:
> >> handle_qmp_command() reports JSON syntax errors right away.  This is
> >> wrong when OOB is enabled, because the errors can "jump the queue"
> >> then.
> >>
> >> The previous commit fixed the same bug for semantic errors, by
> >> delaying the checking until dispatch.  We can't delay the checking, so
> >> delay the reporting.
> >>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> Message-Id: <20180703085358.13941-19-armbru@redhat.com>
> >
> > I'm observing a qemu crash in qemu-iotests 153 (which does however not
> > seem to make the test case fail). git bisect points me to this patch.
> >
> > I'm getting output like this:
> >
> > *** Error in `/home/kwolf/source/qemu/tests/qemu-iotests/qemu': free(): invalid pointer: 0x0000555f7870f7e0 ***
> > ======= Backtrace: =========
> > /lib64/libc.so.6(+0x7cbac)[0x7fa9b29a2bac]
> > /lib64/libc.so.6(+0x87a59)[0x7fa9b29ada59]
> > /lib64/libc.so.6(cfree+0x16e)[0x7fa9b29b33be]
> > /lib64/libglib-2.0.so.0(g_free+0xe)[0x7fa9ce462b4e]
> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6eb9dc)[0x555f76f489dc]
> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x30ae4b)[0x555f76b67e4b]
> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x311558)[0x555f76b6e558]
> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e2d4e)[0x555f76f3fd4e]
> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e5fa0)[0x555f76f42fa0]
> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e2c2e)[0x555f76f3fc2e]
> > /lib64/libglib-2.0.so.0(g_main_context_dispatch+0x157)[0x7fa9ce45d257]
> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e526e)[0x555f76f4226e]
> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x42349e)[0x555f76c8049e]
> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x2c27ef)[0x555f76b1f7ef]
> > /lib64/libc.so.6(__libc_start_main+0xea)[0x7fa9b294688a]
> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x2c5b8a)[0x555f76b22b8a]
> >
> > Interestingly, this doesn't want to produce a core dump for me, so no
> > backtrace with usable function names here. But I assume that you can
> > easily reproduce this yourself.
> >
> 
> Looks like the double-free regression, you could try: "[PATCH]
> monitor: fix double-free of request error"

Thanks, that does fix it. Looks like it missed -rc0, though?

Kevin

Re: [Qemu-devel] [PULL v2 18/32] qmp: Don't let JSON errors jump the queue
Posted by Markus Armbruster 7 years, 3 months ago
Kevin Wolf <kwolf@redhat.com> writes:

> Am 10.07.2018 um 16:02 hat Marc-André Lureau geschrieben:
>> Hi
>> 
>> On Tue, Jul 10, 2018 at 3:20 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> > Am 03.07.2018 um 23:35 hat Markus Armbruster geschrieben:
>> >> handle_qmp_command() reports JSON syntax errors right away.  This is
>> >> wrong when OOB is enabled, because the errors can "jump the queue"
>> >> then.
>> >>
>> >> The previous commit fixed the same bug for semantic errors, by
>> >> delaying the checking until dispatch.  We can't delay the checking, so
>> >> delay the reporting.
>> >>
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> Reviewed-by: Eric Blake <eblake@redhat.com>
>> >> Message-Id: <20180703085358.13941-19-armbru@redhat.com>
>> >
>> > I'm observing a qemu crash in qemu-iotests 153 (which does however not
>> > seem to make the test case fail). git bisect points me to this patch.
>> >
>> > I'm getting output like this:
>> >
>> > *** Error in `/home/kwolf/source/qemu/tests/qemu-iotests/qemu': free(): invalid pointer: 0x0000555f7870f7e0 ***
>> > ======= Backtrace: =========
>> > /lib64/libc.so.6(+0x7cbac)[0x7fa9b29a2bac]
>> > /lib64/libc.so.6(+0x87a59)[0x7fa9b29ada59]
>> > /lib64/libc.so.6(cfree+0x16e)[0x7fa9b29b33be]
>> > /lib64/libglib-2.0.so.0(g_free+0xe)[0x7fa9ce462b4e]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6eb9dc)[0x555f76f489dc]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x30ae4b)[0x555f76b67e4b]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x311558)[0x555f76b6e558]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e2d4e)[0x555f76f3fd4e]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e5fa0)[0x555f76f42fa0]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e2c2e)[0x555f76f3fc2e]
>> > /lib64/libglib-2.0.so.0(g_main_context_dispatch+0x157)[0x7fa9ce45d257]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e526e)[0x555f76f4226e]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x42349e)[0x555f76c8049e]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x2c27ef)[0x555f76b1f7ef]
>> > /lib64/libc.so.6(__libc_start_main+0xea)[0x7fa9b294688a]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x2c5b8a)[0x555f76b22b8a]
>> >
>> > Interestingly, this doesn't want to produce a core dump for me, so no
>> > backtrace with usable function names here. But I assume that you can
>> > easily reproduce this yourself.
>> >
>> 
>> Looks like the double-free regression, you could try: "[PATCH]
>> monitor: fix double-free of request error"
>
> Thanks, that does fix it. Looks like it missed -rc0, though?

Yes.  I'll work on a pull request for -rc1.