From 62815d85aed71eff7b6c3a524705180fb04f5d30 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 23 Aug 2018 18:40:01 +0200 Subject: json: Redesign the callback to consume JSON values The classical way to structure parser and lexer is to have the client call the parser to get an abstract syntax tree, the parser call the lexer to get the next token, and the lexer call some function to get input characters. Another way to structure them would be to have the client feed characters to the lexer, the lexer feed tokens to the parser, and the parser feed abstract syntax trees to some callback provided by the client. This way is more easily integrated into an event loop that dispatches input characters as they arrive. Our JSON parser is kind of between the two. The lexer feeds tokens to a "streamer" instead of a real parser. The streamer accumulates tokens until it got the sequence of tokens that comprise a single JSON value (it counts curly braces and square brackets to decide). It feeds those token sequences to a callback provided by the client. The callback passes each token sequence to the parser, and gets back an abstract syntax tree. I figure it was done that way to make a straightforward recursive descent parser possible. "Get next token" becomes "pop the first token off the token sequence". Drawback: we need to store a complete token sequence. Each token eats 13 + input characters + malloc overhead bytes. Observations: 1. This is not the only way to use recursive descent. If we replaced "get next token" by a coroutine yield, we could do without a streamer. 2. The lexer reports errors by passing a JSON_ERROR token to the streamer. This communicates the offending input characters and their location, but no more. 3. The streamer reports errors by passing a null token sequence to the callback. The (already poor) lexical error information is thrown away. 4. Having the callback receive a token sequence duplicates the code to convert token sequence to abstract syntax tree in every callback. 5. Known bug: the streamer silently drops incomplete token sequences. This commit rectifies 4. by lifting the call of the parser from the callbacks into the streamer. Later commits will address 3. and 5. The lifting removes a bug from qjson.c's parse_json(): it passed a pointer to a non-null Error * in certain cases, as demonstrated by check-qjson.c. json_parser_parse() is now unused. It's a stupid wrapper around json_parser_parse_err(). Drop it, and rename json_parser_parse_err() to json_parser_parse(). Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180823164025.12553-35-armbru@redhat.com> --- monitor.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) (limited to 'monitor.c') diff --git a/monitor.c b/monitor.c index 94f673511b..08f799a7bb 100644 --- a/monitor.c +++ b/monitor.c @@ -59,7 +59,6 @@ #include "qapi/qmp/qstring.h" #include "qapi/qmp/qjson.h" #include "qapi/qmp/json-streamer.h" -#include "qapi/qmp/json-parser.h" #include "qapi/qmp/qlist.h" #include "qom/object_interfaces.h" #include "trace-root.h" @@ -4256,18 +4255,15 @@ static void monitor_qmp_bh_dispatcher(void *data) #define QMP_REQ_QUEUE_LEN_MAX (8) -static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) +static void handle_qmp_command(void *opaque, QObject *req, Error *err) { - QObject *req, *id = NULL; + Monitor *mon = opaque; + QObject *id = NULL; QDict *qdict; - MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser); - Monitor *mon = container_of(mon_qmp, Monitor, qmp); - Error *err = NULL; QMPRequest *req_obj; - req = json_parser_parse_err(tokens, NULL, &err); if (!req && !err) { - /* json_parser_parse_err() sucks: can fail without setting @err */ + /* json_parser_parse() sucks: can fail without setting @err */ error_setg(&err, QERR_JSON_PARSING); } @@ -4465,7 +4461,8 @@ static void monitor_qmp_event(void *opaque, int event) monitor_qmp_response_flush(mon); monitor_qmp_cleanup_queues(mon); json_message_parser_destroy(&mon->qmp.parser); - json_message_parser_init(&mon->qmp.parser, handle_qmp_command); + json_message_parser_init(&mon->qmp.parser, handle_qmp_command, + mon, NULL); mon_refcount--; monitor_fdsets_cleanup(); break; @@ -4683,7 +4680,8 @@ void monitor_init(Chardev *chr, int flags) if (monitor_is_qmp(mon)) { qemu_chr_fe_set_echo(&mon->chr, true); - json_message_parser_init(&mon->qmp.parser, handle_qmp_command); + json_message_parser_init(&mon->qmp.parser, handle_qmp_command, + mon, NULL); if (mon->use_io_thread) { /* * Make sure the old iowatch is gone. It's possible when -- cgit v1.2.3-55-g7522 From 84a56f38b23440cb3127eaffe4e495826a29f18c Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 23 Aug 2018 18:40:06 +0200 Subject: json: Pass lexical errors and limit violations to callback The callback to consume JSON values takes QObject *json, Error *err. If both are null, the callback is supposed to make up an error by itself. This sucks. qjson.c's consume_json() neglects to do so, which makes qobject_from_json() null instead of failing. I consider that a bug. The culprit is json_message_process_token(): it passes two null pointers when it runs into a lexical error or a limit violation. Fix it to pass a proper Error object then. Update the callbacks: * monitor.c's handle_qmp_command(): the code to make up an error is now dead, drop it. * qga/main.c's process_event(): lumps the "both null" case together with the "not a JSON object" case. The former is now gone. The error message "Invalid JSON syntax" is misleading for the latter. Improve it to "Input must be a JSON object". * qobject/qjson.c's consume_json(): no update; check-qjson demonstrates qobject_from_json() now sets an error on lexical errors, but still doesn't on some other errors. * tests/libqtest.c's qmp_response(): the Error object is now reliable, so use it to improve the error message. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180823164025.12553-40-armbru@redhat.com> --- include/qapi/qmp/qerror.h | 3 --- monitor.c | 5 +---- qga/main.c | 3 ++- qobject/json-lexer.c | 3 +-- qobject/json-streamer.c | 22 ++++++++++++++++------ tests/check-qjson.c | 15 ++++++++------- tests/libqtest.c | 7 +++++-- 7 files changed, 33 insertions(+), 25 deletions(-) (limited to 'monitor.c') diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h index c82360f429..145571f618 100644 --- a/include/qapi/qmp/qerror.h +++ b/include/qapi/qmp/qerror.h @@ -61,9 +61,6 @@ #define QERR_IO_ERROR \ "An IO error has occurred" -#define QERR_JSON_PARSING \ - "Invalid JSON syntax" - #define QERR_MIGRATION_ACTIVE \ "There's a migration process in progress" diff --git a/monitor.c b/monitor.c index 08f799a7bb..3dbdcb5190 100644 --- a/monitor.c +++ b/monitor.c @@ -4262,10 +4262,7 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err) QDict *qdict; QMPRequest *req_obj; - if (!req && !err) { - /* json_parser_parse() sucks: can fail without setting @err */ - error_setg(&err, QERR_JSON_PARSING); - } + assert(!req != !err); qdict = qobject_to(QDict, req); if (qdict) { diff --git a/qga/main.c b/qga/main.c index 2fc49d00d8..b74e1241ef 100644 --- a/qga/main.c +++ b/qga/main.c @@ -603,12 +603,13 @@ static void process_event(void *opaque, QObject *obj, Error *err) int ret; g_debug("process_event: called"); + assert(!obj != !err); if (err) { goto err; } req = qobject_to(QDict, obj); if (!req) { - error_setg(&err, QERR_JSON_PARSING); + error_setg(&err, "Input must be a JSON object"); goto err; } if (!qdict_haskey(req, "execute")) { diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c index 96fe13621d..7c31c2c8ff 100644 --- a/qobject/json-lexer.c +++ b/qobject/json-lexer.c @@ -334,8 +334,7 @@ static void json_lexer_feed_char(JSONLexer *lexer, char ch, bool flush) /* XXX: To avoid having previous bad input leaving the parser in an * unresponsive state where we consume unpredictable amounts of * subsequent "good" input, percolate this error state up to the - * tokenizer/parser by forcing a NULL object to be emitted, then - * reset state. + * parser by emitting a JSON_ERROR token, then reset lexer state. * * Also note that this handling is required for reliable channel * negotiation between QMP and the guest agent, since chr(0xFF) diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c index a373e0114a..e372ecc895 100644 --- a/qobject/json-streamer.c +++ b/qobject/json-streamer.c @@ -13,6 +13,7 @@ #include "qemu/osdep.h" #include "qemu-common.h" +#include "qapi/error.h" #include "qapi/qmp/json-lexer.h" #include "qapi/qmp/json-parser.h" #include "qapi/qmp/json-streamer.h" @@ -57,6 +58,7 @@ void json_message_process_token(JSONLexer *lexer, GString *input, parser->bracket_count--; break; case JSON_ERROR: + error_setg(&err, "JSON parse error, stray '%s'", input->str); goto out_emit; default: break; @@ -82,12 +84,20 @@ void json_message_process_token(JSONLexer *lexer, GString *input, goto out_emit; } - if (parser->token_size > MAX_TOKEN_SIZE || - g_queue_get_length(parser->tokens) > MAX_TOKEN_COUNT || - parser->bracket_count + parser->brace_count > MAX_NESTING) { - /* Security consideration, we limit total memory allocated per object - * and the maximum recursion depth that a message can force. - */ + /* + * Security consideration, we limit total memory allocated per object + * and the maximum recursion depth that a message can force. + */ + if (parser->token_size > MAX_TOKEN_SIZE) { + error_setg(&err, "JSON token size limit exceeded"); + goto out_emit; + } + if (g_queue_get_length(parser->tokens) > MAX_TOKEN_COUNT) { + error_setg(&err, "JSON token count limit exceeded"); + goto out_emit; + } + if (parser->bracket_count + parser->brace_count > MAX_NESTING) { + error_setg(&err, "JSON nesting depth limit exceeded"); goto out_emit; } diff --git a/tests/check-qjson.c b/tests/check-qjson.c index 604886a1a2..d6fda0786f 100644 --- a/tests/check-qjson.c +++ b/tests/check-qjson.c @@ -1021,6 +1021,7 @@ static void interpolation_unknown(void) } g_test_trap_subprocess(NULL, 0, 0); g_test_trap_assert_failed(); + g_test_trap_assert_stderr("*Unexpected error*stray '%x'*"); } static void interpolation_string(void) @@ -1296,11 +1297,11 @@ static void junk_input(void) QObject *obj; obj = qobject_from_json("@", &err); - g_assert(!err); /* BUG */ + error_free_or_abort(&err); g_assert(obj == NULL); obj = qobject_from_json("{\x01", &err); - g_assert(!err); /* BUG */ + error_free_or_abort(&err); g_assert(obj == NULL); obj = qobject_from_json("[0\xFF]", &err); @@ -1308,11 +1309,11 @@ static void junk_input(void) g_assert(obj == NULL); obj = qobject_from_json("00", &err); - g_assert(!err); /* BUG */ + error_free_or_abort(&err); g_assert(obj == NULL); obj = qobject_from_json("[1e", &err); - g_assert(!err); /* BUG */ + error_free_or_abort(&err); g_assert(obj == NULL); obj = qobject_from_json("truer", &err); @@ -1324,7 +1325,7 @@ static void unterminated_string(void) { Error *err = NULL; QObject *obj = qobject_from_json("\"abc", &err); - g_assert(!err); /* BUG */ + error_free_or_abort(&err); g_assert(obj == NULL); } @@ -1332,7 +1333,7 @@ static void unterminated_sq_string(void) { Error *err = NULL; QObject *obj = qobject_from_json("'abc", &err); - g_assert(!err); /* BUG */ + error_free_or_abort(&err); g_assert(obj == NULL); } @@ -1340,7 +1341,7 @@ static void unterminated_escape(void) { Error *err = NULL; QObject *obj = qobject_from_json("\"abc\\\"", &err); - g_assert(!err); /* BUG */ + error_free_or_abort(&err); g_assert(obj == NULL); } diff --git a/tests/libqtest.c b/tests/libqtest.c index 1f3b0cb1b1..5973a67652 100644 --- a/tests/libqtest.c +++ b/tests/libqtest.c @@ -450,8 +450,11 @@ static void qmp_response(void *opaque, QObject *obj, Error *err) { QMPResponseParser *qmp = opaque; - if (!obj) { - fprintf(stderr, "QMP JSON response parsing failed\n"); + assert(!obj != !err); + + if (err) { + error_prepend(&err, "QMP JSON response parsing failed: "); + error_report_err(err); abort(); } -- cgit v1.2.3-55-g7522 From 86cdf9ec8dec2763702cc52fa412d108a5dc9608 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 23 Aug 2018 18:40:20 +0200 Subject: json: Clean up headers The JSON parser has three public headers, json-lexer.h, json-parser.h, json-streamer.h. They all contain stuff that is of no interest outside qobject/json-*.c. Collect the public interface in include/qapi/qmp/json-parser.h, and everything else in qobject/json-parser-int.h. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20180823164025.12553-54-armbru@redhat.com> --- include/qapi/qmp/json-lexer.h | 50 ------------------------------------- include/qapi/qmp/json-parser.h | 36 +++++++++++++++++++++------ include/qapi/qmp/json-streamer.h | 46 ---------------------------------- monitor.c | 2 +- qga/main.c | 2 +- qobject/json-lexer.c | 3 +-- qobject/json-parser-int.h | 54 ++++++++++++++++++++++++++++++++++++++++ qobject/json-parser.c | 4 +-- qobject/json-streamer.c | 4 +-- qobject/qjson.c | 2 +- tests/libqtest.c | 2 +- 11 files changed, 90 insertions(+), 115 deletions(-) delete mode 100644 include/qapi/qmp/json-lexer.h delete mode 100644 include/qapi/qmp/json-streamer.h create mode 100644 qobject/json-parser-int.h (limited to 'monitor.c') diff --git a/include/qapi/qmp/json-lexer.h b/include/qapi/qmp/json-lexer.h deleted file mode 100644 index 508fc7bdaf..0000000000 --- a/include/qapi/qmp/json-lexer.h +++ /dev/null @@ -1,50 +0,0 @@ -/* - * JSON lexer - * - * Copyright IBM, Corp. 2009 - * - * Authors: - * Anthony Liguori - * - * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. - * See the COPYING.LIB file in the top-level directory. - * - */ - -#ifndef QEMU_JSON_LEXER_H -#define QEMU_JSON_LEXER_H - - -typedef enum json_token_type { - JSON_MIN = 100, - JSON_LCURLY = JSON_MIN, - JSON_RCURLY, - JSON_LSQUARE, - JSON_RSQUARE, - JSON_COLON, - JSON_COMMA, - JSON_INTEGER, - JSON_FLOAT, - JSON_KEYWORD, - JSON_STRING, - JSON_INTERP, - JSON_SKIP, - JSON_ERROR, - JSON_END_OF_INPUT, -} JSONTokenType; - -typedef struct JSONLexer { - int start_state, state; - GString *token; - int x, y; -} JSONLexer; - -void json_lexer_init(JSONLexer *lexer, bool enable_interpolation); - -void json_lexer_feed(JSONLexer *lexer, const char *buffer, size_t size); - -void json_lexer_flush(JSONLexer *lexer); - -void json_lexer_destroy(JSONLexer *lexer); - -#endif diff --git a/include/qapi/qmp/json-parser.h b/include/qapi/qmp/json-parser.h index 55f75954c3..7345a9bd5c 100644 --- a/include/qapi/qmp/json-parser.h +++ b/include/qapi/qmp/json-parser.h @@ -1,5 +1,5 @@ /* - * JSON Parser + * JSON Parser * * Copyright IBM, Corp. 2009 * @@ -11,14 +11,36 @@ * */ -#ifndef QEMU_JSON_PARSER_H -#define QEMU_JSON_PARSER_H +#ifndef QAPI_QMP_JSON_PARSER_H +#define QAPI_QMP_JSON_PARSER_H -#include "qapi/qmp/json-lexer.h" +typedef struct JSONLexer { + int start_state, state; + GString *token; + int x, y; +} JSONLexer; -typedef struct JSONToken JSONToken; +typedef struct JSONMessageParser { + void (*emit)(void *opaque, QObject *json, Error *err); + void *opaque; + va_list *ap; + JSONLexer lexer; + int brace_count; + int bracket_count; + GQueue tokens; + uint64_t token_size; +} JSONMessageParser; -JSONToken *json_token(JSONTokenType type, int x, int y, GString *tokstr); -QObject *json_parser_parse(GQueue *tokens, va_list *ap, Error **errp); +void json_message_parser_init(JSONMessageParser *parser, + void (*emit)(void *opaque, QObject *json, + Error *err), + void *opaque, va_list *ap); + +void json_message_parser_feed(JSONMessageParser *parser, + const char *buffer, size_t size); + +void json_message_parser_flush(JSONMessageParser *parser); + +void json_message_parser_destroy(JSONMessageParser *parser); #endif diff --git a/include/qapi/qmp/json-streamer.h b/include/qapi/qmp/json-streamer.h deleted file mode 100644 index 29950ac37c..0000000000 --- a/include/qapi/qmp/json-streamer.h +++ /dev/null @@ -1,46 +0,0 @@ -/* - * JSON streaming support - * - * Copyright IBM, Corp. 2009 - * - * Authors: - * Anthony Liguori - * - * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. - * See the COPYING.LIB file in the top-level directory. - * - */ - -#ifndef QEMU_JSON_STREAMER_H -#define QEMU_JSON_STREAMER_H - -#include "qapi/qmp/json-lexer.h" - -typedef struct JSONMessageParser -{ - void (*emit)(void *opaque, QObject *json, Error *err); - void *opaque; - va_list *ap; - JSONLexer lexer; - int brace_count; - int bracket_count; - GQueue tokens; - uint64_t token_size; -} JSONMessageParser; - -void json_message_process_token(JSONLexer *lexer, GString *input, - JSONTokenType type, int x, int y); - -void json_message_parser_init(JSONMessageParser *parser, - void (*emit)(void *opaque, QObject *json, - Error *err), - void *opaque, va_list *ap); - -void json_message_parser_feed(JSONMessageParser *parser, - const char *buffer, size_t size); - -void json_message_parser_flush(JSONMessageParser *parser); - -void json_message_parser_destroy(JSONMessageParser *parser); - -#endif diff --git a/monitor.c b/monitor.c index 3dbdcb5190..021c11b1bf 100644 --- a/monitor.c +++ b/monitor.c @@ -58,7 +58,7 @@ #include "qapi/qmp/qnum.h" #include "qapi/qmp/qstring.h" #include "qapi/qmp/qjson.h" -#include "qapi/qmp/json-streamer.h" +#include "qapi/qmp/json-parser.h" #include "qapi/qmp/qlist.h" #include "qom/object_interfaces.h" #include "trace-root.h" diff --git a/qga/main.c b/qga/main.c index b74e1241ef..6d70242d05 100644 --- a/qga/main.c +++ b/qga/main.c @@ -18,7 +18,7 @@ #include #include #endif -#include "qapi/qmp/json-streamer.h" +#include "qapi/qmp/json-parser.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qjson.h" #include "qapi/qmp/qstring.h" diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c index 06ec67dc45..e1745a3d95 100644 --- a/qobject/json-lexer.c +++ b/qobject/json-lexer.c @@ -12,8 +12,7 @@ */ #include "qemu/osdep.h" -#include "qapi/qmp/json-lexer.h" -#include "qapi/qmp/json-streamer.h" +#include "json-parser-int.h" #define MAX_TOKEN_SIZE (64ULL << 20) diff --git a/qobject/json-parser-int.h b/qobject/json-parser-int.h new file mode 100644 index 0000000000..ceaa890ec6 --- /dev/null +++ b/qobject/json-parser-int.h @@ -0,0 +1,54 @@ +/* + * JSON Parser + * + * Copyright IBM, Corp. 2009 + * + * Authors: + * Anthony Liguori + * + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. + * See the COPYING.LIB file in the top-level directory. + * + */ + +#ifndef JSON_PARSER_INT_H +#define JSON_PARSER_INT_H + +#include "qapi/qmp/json-parser.h" + + +typedef enum json_token_type { + JSON_MIN = 100, + JSON_LCURLY = JSON_MIN, + JSON_RCURLY, + JSON_LSQUARE, + JSON_RSQUARE, + JSON_COLON, + JSON_COMMA, + JSON_INTEGER, + JSON_FLOAT, + JSON_KEYWORD, + JSON_STRING, + JSON_INTERP, + JSON_SKIP, + JSON_ERROR, + JSON_END_OF_INPUT, +} JSONTokenType; + +typedef struct JSONToken JSONToken; + +/* json-lexer.c */ +void json_lexer_init(JSONLexer *lexer, bool enable_interpolation); +void json_lexer_feed(JSONLexer *lexer, const char *buffer, size_t size); +void json_lexer_flush(JSONLexer *lexer); +void json_lexer_destroy(JSONLexer *lexer); + +/* json-streamer.c */ +void json_message_process_token(JSONLexer *lexer, GString *input, + JSONTokenType type, int x, int y); + +/* json-parser.c */ +JSONToken *json_token(JSONTokenType type, int x, int y, GString *tokstr); +QObject *json_parser_parse(GQueue *tokens, va_list *ap, Error **errp); + +#endif diff --git a/qobject/json-parser.c b/qobject/json-parser.c index a247875f14..7449684f1c 100644 --- a/qobject/json-parser.c +++ b/qobject/json-parser.c @@ -22,9 +22,7 @@ #include "qapi/qmp/qnull.h" #include "qapi/qmp/qnum.h" #include "qapi/qmp/qstring.h" -#include "qapi/qmp/json-parser.h" -#include "qapi/qmp/json-lexer.h" -#include "qapi/qmp/json-streamer.h" +#include "json-parser-int.h" struct JSONToken { JSONTokenType type; diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c index da53e770e9..47dd7ea576 100644 --- a/qobject/json-streamer.c +++ b/qobject/json-streamer.c @@ -13,9 +13,7 @@ #include "qemu/osdep.h" #include "qapi/error.h" -#include "qapi/qmp/json-lexer.h" -#include "qapi/qmp/json-parser.h" -#include "qapi/qmp/json-streamer.h" +#include "json-parser-int.h" #define MAX_TOKEN_SIZE (64ULL << 20) #define MAX_TOKEN_COUNT (2ULL << 20) diff --git a/qobject/qjson.c b/qobject/qjson.c index b9ccae2c2a..db36101f3b 100644 --- a/qobject/qjson.c +++ b/qobject/qjson.c @@ -13,7 +13,7 @@ #include "qemu/osdep.h" #include "qapi/error.h" -#include "qapi/qmp/json-streamer.h" +#include "qapi/qmp/json-parser.h" #include "qapi/qmp/qjson.h" #include "qapi/qmp/qbool.h" #include "qapi/qmp/qdict.h" diff --git a/tests/libqtest.c b/tests/libqtest.c index 5973a67652..d635c5bea0 100644 --- a/tests/libqtest.c +++ b/tests/libqtest.c @@ -24,7 +24,7 @@ #include "qemu-common.h" #include "qemu/cutils.h" #include "qapi/error.h" -#include "qapi/qmp/json-streamer.h" +#include "qapi/qmp/json-parser.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qjson.h" #include "qapi/qmp/qlist.h" -- cgit v1.2.3-55-g7522