Re: [Qemu-devel] [PATCH 38/56] json: Pass lexical errors and limit violations to callback

2018-08-13 Thread Eric Blake

On 08/08/2018 07:03 AM, Markus Armbruster wrote:

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() & friends return 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:




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


Bonus - one less of these annoying defines.

Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



[Qemu-devel] [PATCH 38/56] json: Pass lexical errors and limit violations to callback

2018-08-08 Thread Markus Armbruster
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() & friends return 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 
---
 include/qapi/qmp/qerror.h |  3 ---
 monitor.c |  5 +
 qga/main.c|  3 ++-
 qobject/json-streamer.c   | 22 --
 tests/check-qjson.c   | 14 +++---
 tests/libqtest.c  |  7 +--
 6 files changed, 31 insertions(+), 23 deletions(-)

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 71658d9905..dc0ed8df92 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4251,10 +4251,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-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 4c4afcf691..895be489b3 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -1247,11 +1247,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 = qobj