Re: [Qemu-devel] [PATCH v2 13/18] json-parser: set an error if parsing returned NULL

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Let's make json_parser_parse_err() suck less, and simplify caller
> error handling.

Let's state what's wrong with it first, like this:

  json_parser_parse_err() returns null on empty input and on parse error.
  In the latter case, it sometimes, but not always sets an error.  This
  sucks.  Fix it to always set an error, and simplify callers.

>  * monitor.c handle_qmp_command(): drop workaround
>
>  * qga/main.c process_event(): improve error report, QERR_JSON_PARSING
>    case is handled by json_parser_parse_err() now.
>
>  * qobject/json-parser.c json_parser_parse(): Ignores the error.
>
>  * qobject/qjson.c qobject_from_jsonv() via parse_json()
>   - qobject_from_json()
>     ~ block.c parse_json_filename() - removed workaround
>     ~ block/rbd.c - abort (generated json - should never fail)

Actually, we improve the failure from "qlist_size() crashes" to "the new
error_setg() aborts".

>     ~ qapi/qobject-input-visitor.c - removed workaround
>     ~ tests/check-qjson.c - assert or crash
>     ~ tests/test-visitor-serialization.c - assert or crash
>
>   - qobject_from_jsonf()
>     Now dies in error_handle_fatal() instead of the assertion.
>     Only used in tests, ok.
>
>   - tests/test-qobject-input-visitor.c
>   - tests/libqtest.c qmp_fd_sendv()
>     Passes &error_abort, does nothing when qobject_from_jsonv() returns
>     null.  The fix makes this catch invalid JSON instead of silently
>     ignoring it.
>
> Update the function documentation for possible return values.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  block.c                      |  5 -----
>  monitor.c                    |  4 ----
>  qapi/qobject-input-visitor.c |  5 -----
>  qga/main.c                   |  2 +-
>  qobject/json-parser.c        | 16 +++++++++++++++-
>  5 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/block.c b/block.c
> index a2fe05ea96..42eaa8b7dc 100644
> --- a/block.c
> +++ b/block.c
> @@ -1478,11 +1478,6 @@ static QDict *parse_json_filename(const char *filename, Error **errp)
>  
>      options_obj = qobject_from_json(filename, errp);
>      if (!options_obj) {
> -        /* Work around qobject_from_json() lossage TODO fix that */
> -        if (errp && !*errp) {
> -            error_setg(errp, "Could not parse the JSON options");
> -            return NULL;
> -        }
>          error_prepend(errp, "Could not parse the JSON options: ");

Crashes if qobject_from_json() returns null without setting an error,
e.g. when input is empty:

    (gdb) p parse_json_filename("json: ", &error_abort)
    [Thread 0x7fffd5bb8700 (LWP 10318) exited]

    Thread 1 "upstream-qemu" received signal SIGSEGV, Segmentation fault.
    error_vprepend (errp=0x555556850110 <error_abort>, 
        fmt=0x555556044a30 "Could not parse the JSON options: ", ap=0x7fffffffd9b0)
        at /work/armbru/qemu/util/error.c:136
    136	    g_string_append(newmsg, (*errp)->msg);


>          return NULL;
>      }
> diff --git a/monitor.c b/monitor.c
> index 2abb3c2fc7..5a41a230b9 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4114,10 +4114,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>      QMPRequest *req_obj;
>  
>      req = json_parser_parse(tokens, NULL, &err);
> -    if (!req && !err) {
> -        /* json_parser_parse() sucks: can fail without setting @err */
> -        error_setg(&err, QERR_JSON_PARSING);
> -    }
>  
>      qdict = qobject_to(QDict, req);
>      if (qdict) {

What if json_parser_parse() returns null without setting an error?  It
can when @tokens is null.  Hmm...

    $ gdb --args upstream-qemu -nodefaults -S -display none -qmp stdio
    [...]
    (gdb) r
    [...]
    {"QMP": {"version": {"qemu": {"micro": 90, "minor": 12, "major": 2}, "package": "v3.0.0-rc1-23-ga18a0aefe5"}, "capabilities": []}}
    [New Thread 0x7fffd5c38700 (LWP 11878)]
    @
    upstream-qemu: /work/armbru/qemu/monitor.c:4088: monitor_qmp_bh_dispatcher: Assertion `req_obj->err' failed.

    Thread 1 "upstream-qemu" received signal SIGABRT, Aborted.
    0x00007fffec607feb in raise () from /lib64/libc.so.6
    [...]
    (gdb) bt
    #0  0x00007fffec607feb in raise () at /lib64/libc.so.6
    #1  0x00007fffec5f25c1 in abort () at /lib64/libc.so.6
    #2  0x00007fffec5f2491 in _nl_load_domain.cold.0 () at /lib64/libc.so.6
    #3  0x00007fffec600752 in  () at /lib64/libc.so.6
    #4  0x0000555555874236 in monitor_qmp_bh_dispatcher (data=0x0)
        at /work/armbru/qemu/monitor.c:4088
    [...]

Crash on lexical error.  See review of json_parser_parse() below.

We lack test coverage.  Patch appended.

> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index da57f4cc24..3e88b27f9e 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -725,11 +725,6 @@ Visitor *qobject_input_visitor_new_str(const char *str,
>      if (is_json) {
>          obj = qobject_from_json(str, errp);
>          if (!obj) {
> -            /* Work around qobject_from_json() lossage TODO fix that */
> -            if (errp && !*errp) {
> -                error_setg(errp, "JSON parse error");
> -                return NULL;
> -            }
>              return NULL;
>          }
>          args = qobject_to(QDict, obj);

Likewise, what if json_parser_parse() returns null without setting an
error?

> diff --git a/qga/main.c b/qga/main.c
> index 043f7c3ead..9032bb0c7a 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -614,7 +614,7 @@ static void process_event(JSONMessageParser *parser, GQueue *tokens)
>      }
>      req = qobject_to(QDict, obj);
>      if (!req) {
> -        error_setg(&err, QERR_JSON_PARSING);
> +        error_setg(&err, "Input must be a JSON object");

Commit message explains why this is appropriate.  Good.

>          goto err;
>      }
>      if (!qdict_haskey(req, "execute")) {
> diff --git a/qobject/json-parser.c b/qobject/json-parser.c
> index 0c0b478149..c3b0c216cf 100644
> --- a/qobject/json-parser.c
> +++ b/qobject/json-parser.c
> @@ -24,6 +24,7 @@
>  #include "qapi/qmp/json-parser.h"
>  #include "qapi/qmp/json-lexer.h"
>  #include "qapi/qmp/json-streamer.h"
> +#include "qapi/qmp/qerror.h"
>  
>  typedef struct JSONParserContext
>  {
> @@ -548,6 +549,14 @@ static QObject *parse_value(JSONParserContext *ctxt, va_list *ap)
>      }
>  }
>  
> +/**
> + * json_parser_parse:
> + *
> + * If @tokens is null, return null.
> + * Else if @tokens parse okay, return the parse tree.
> + * Else set an error and return null.
> + *
> + **/

We end doc comments with an unadorned */, in accordance with the GTK-Doc
manual.

The @tokens argument always comes from json_message_process_token():

    out_emit_bad:
        /*
         * Clear out token list and tell the parser to emit an error
         * indication by passing it a NULL list
         */
        json_message_free_tokens(parser);
    out_emit:
        /* send current list of tokens to parser and reset tokenizer */
        parser->brace_count = 0;
        parser->bracket_count = 0;
        /* parser->emit takes ownership of parser->tokens.  Remove our own
         * reference to parser->tokens before handing it out to parser->emit.
         */
        tokens = parser->tokens;
        parser->tokens = g_queue_new();
        parser->emit(parser, tokens);

parser->tokens is null exactly when we pass out_emit_bad.

Thus, the intent of null @tokens is to signal a lexical error.

Aside: there's no way to pass information on the error along with this
signal, but that's not this patch's problem.

>  QObject *json_parser_parse(GQueue *tokens, va_list *ap, Error **errp)
>  {
>      JSONParserContext ctxt = { .buf = tokens };
> @@ -559,7 +568,12 @@ QObject *json_parser_parse(GQueue *tokens, va_list *ap, Error **errp)
       QObject *result;

       if (!tokens) {
           return NULL;
       }

This does not match the intent declared in json_message_process_token().

>  
>      result = parse_value(&ctxt, ap);
>  
> -    error_propagate(errp, ctxt.err);
> +    if (!result && !ctxt.err) {
> +        /* TODO: improve error reporting */

I'd point out the issue explicitly here: parse_value() can return null
without setting ctxt.err.  No need to make more specific claims.
Although A quick eye over suggests parse_escape() is to blame.

> +        error_setg(errp, QERR_JSON_PARSING);
> +    } else {
> +        error_propagate(errp, ctxt.err);
> +    }
>  
>      g_queue_free_full(ctxt.buf, g_free);
>      g_free(ctxt.current);

I hope the JSON parser simplifications I have in mind will help take
care of this patch's issues, too.


>From f469aabeb5950d4891fbf5fa3a76ea333f540848 Mon Sep 17 00:00:00 2001
From: Markus Armbruster <armbru@redhat.com>
Date: Mon, 23 Jul 2018 10:08:29 +0200
Subject: [PATCH] check-qjson: Cover blank and lexically erroneous input

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/check-qjson.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index d0144ba93c..3acd5a37a3 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -1306,8 +1306,27 @@ static void simple_varargs(void)
 
 static void empty_input(void)
 {
-    const char *empty = "";
-    QObject *obj = qobject_from_json(empty, &error_abort);
+    QObject *obj = qobject_from_json("", &error_abort);
+    g_assert(obj == NULL);
+}
+
+static void blank_input(void)
+{
+    QObject *obj = qobject_from_json("", &error_abort);
+    g_assert(obj == NULL);
+}
+
+static void junk_input(void)
+{
+    Error *err = NULL;
+    QObject *obj;
+
+    obj = qobject_from_json("@", &err);
+    error_free_or_abort(&err);
+    g_assert(obj == NULL);
+
+    obj = qobject_from_json("{@", &err);
+    error_free_or_abort(&err);
     g_assert(obj == NULL);
 }
 
@@ -1452,7 +1471,9 @@ int main(int argc, char **argv)
 
     g_test_add_func("/varargs/simple_varargs", simple_varargs);
 
-    g_test_add_func("/errors/empty_input", empty_input);
+    g_test_add_func("/errors/empty", empty_input);
+    g_test_add_func("/errors/blank", blank_input);
+    g_test_add_func("/errors/junk", junk_input);
     g_test_add_func("/errors/unterminated/string", unterminated_string);
     g_test_add_func("/errors/unterminated/escape", unterminated_escape);
     g_test_add_func("/errors/unterminated/sq_string", unterminated_sq_string);
-- 
2.17.1