From 5df5fc5b13cac5212482d36e7f3a78951782cfb5 Mon Sep 17 00:00:00 2001 From: Corey Farrell Date: Tue, 25 Sep 2018 14:31:56 -0400 Subject: [PATCH 1/2] json_pack: Improve handling of formats with '?' and '*'. When NULL is received for an optional argument we should not set an error message as this would block later error messages. If NULL is received for a non-optional string we should set has_error. Set has_error for UTF-8 errors to ensure optional strings with UTF-8 errors are not replaced with json_null(). Use 'purpose' argument in NULL error messages of read_string. Add error handling and tests for invalid formats where '+', '#', or '%' is used on an optional string 's?' or 's*'. Fix NULL string error messages to use 'purpose'. Refactor skipping of '*' token, this is now handled by read_string and pack_object_inter. This allows invalid format strings such as 's*#' and 's*+' to produce error messages. Fixes #437 --- src/pack_unpack.c | 74 +++++++++++++++++++++++-------------- test/suites/api/test_pack.c | 49 ++++++++++++++++++++++-- 2 files changed, 93 insertions(+), 30 deletions(-) diff --git a/src/pack_unpack.c b/src/pack_unpack.c index b842772..fc98df4 100644 --- a/src/pack_unpack.c +++ b/src/pack_unpack.c @@ -130,7 +130,7 @@ static json_t *pack(scanner_t *s, va_list *ap); /* ours will be set to 1 if jsonp_free() must be called for the result afterwards */ static char *read_string(scanner_t *s, va_list *ap, - const char *purpose, size_t *out_len, int *ours) + const char *purpose, size_t *out_len, int *ours, int optional) { char t; strbuffer_t strbuff; @@ -147,7 +147,10 @@ static char *read_string(scanner_t *s, va_list *ap, str = va_arg(*ap, const char *); if(!str) { - set_error(s, "", json_error_null_value, "NULL string argument"); + if (!optional) { + set_error(s, "", json_error_null_value, "NULL %s", purpose); + s->has_error = 1; + } return NULL; } @@ -155,11 +158,17 @@ static char *read_string(scanner_t *s, va_list *ap, if(!utf8_check_string(str, length)) { set_error(s, "", json_error_invalid_utf8, "Invalid UTF-8 %s", purpose); + s->has_error = 1; return NULL; } *out_len = length; return (char *)str; + } else if (optional) { + set_error(s, "", json_error_invalid_format, "Cannot use '%c' on optional strings", t); + s->has_error = 1; + + return NULL; } if(strbuffer_init(&strbuff)) { @@ -170,7 +179,7 @@ static char *read_string(scanner_t *s, va_list *ap, while(1) { str = va_arg(*ap, const char *); if(!str) { - set_error(s, "", json_error_null_value, "NULL string argument"); + set_error(s, "", json_error_null_value, "NULL %s", purpose); s->has_error = 1; } @@ -226,6 +235,7 @@ static json_t *pack_object(scanner_t *s, va_list *ap) size_t len; int ours; json_t *value; + char valueOptional; if(!token(s)) { set_error(s, "", json_error_invalid_format, "Unexpected end of format string"); @@ -237,20 +247,21 @@ static json_t *pack_object(scanner_t *s, va_list *ap) goto error; } - key = read_string(s, ap, "object key", &len, &ours); - if (!key) - s->has_error = 1; + key = read_string(s, ap, "object key", &len, &ours, 0); next_token(s); + next_token(s); + valueOptional = token(s); + prev_token(s); + value = pack(s, ap); if(!value) { if(ours) jsonp_free(key); - if(strchr("soO", token(s)) && s->next_token.token == '*') { - next_token(s); - } else { + if(valueOptional != '*') { + set_error(s, "", json_error_null_value, "NULL object value\n"); s->has_error = 1; } @@ -269,8 +280,6 @@ static json_t *pack_object(scanner_t *s, va_list *ap) if(ours) jsonp_free(key); - if(strchr("soO", token(s)) && s->next_token.token == '*') - next_token(s); next_token(s); } @@ -289,6 +298,7 @@ static json_t *pack_array(scanner_t *s, va_list *ap) while(token(s) != ']') { json_t *value; + char valueOptional; if(!token(s)) { set_error(s, "", json_error_invalid_format, "Unexpected end of format string"); @@ -296,11 +306,13 @@ static json_t *pack_array(scanner_t *s, va_list *ap) goto error; } + next_token(s); + valueOptional = token(s); + prev_token(s); + value = pack(s, ap); if(!value) { - if(strchr("soO", token(s)) && s->next_token.token == '*') { - next_token(s); - } else { + if(valueOptional != '*') { s->has_error = 1; } @@ -316,8 +328,6 @@ static json_t *pack_array(scanner_t *s, va_list *ap) s->has_error = 1; } - if(strchr("soO", token(s)) && s->next_token.token == '*') - next_token(s); next_token(s); } @@ -332,23 +342,33 @@ error: static json_t *pack_string(scanner_t *s, va_list *ap) { char *str; + char t; size_t len; int ours; - int nullable; + int optional; next_token(s); - nullable = token(s) == '?'; - if (!nullable) + t = token(s); + optional = t == '?' || t == '*'; + if (!optional) prev_token(s); - str = read_string(s, ap, "string", &len, &ours); - if (!str) { - return nullable ? json_null() : NULL; - } else if (ours) { - return jsonp_stringn_nocheck_own(str, len); - } else { - return json_stringn_nocheck(str, len); + str = read_string(s, ap, "string", &len, &ours, optional); + + if (!str) + return t == '?' && !s->has_error ? json_null() : NULL; + + if (s->has_error) { + if (!ours) + jsonp_free(str); + + return NULL; } + + if (ours) + return jsonp_stringn_nocheck_own(str, len); + + return json_stringn_nocheck(str, len); } static json_t *pack_object_inter(scanner_t *s, va_list *ap, int need_incref) @@ -359,7 +379,7 @@ static json_t *pack_object_inter(scanner_t *s, va_list *ap, int need_incref) next_token(s); ntoken = token(s); - if (ntoken != '?') + if (ntoken != '?' && ntoken != '*') prev_token(s); json = va_arg(*ap, json_t *); diff --git a/test/suites/api/test_pack.c b/test/suites/api/test_pack.c index a1e8e01..7e1c0b5 100644 --- a/test/suites/api/test_pack.c +++ b/test/suites/api/test_pack.c @@ -99,6 +99,16 @@ static void run_tests() fail("json_pack nullable string (NULL case) refcount failed"); json_decref(value); + /* nullable string concatenation */ + if(json_pack_ex(&error, 0, "s?+", "test", "ing")) + fail("json_pack failed to catch invalid format 's?+'"); + check_error(json_error_invalid_format, "Cannot use '+' on optional strings", "", 1, 2, 2); + + /* nullable string with integer length */ + if(json_pack_ex(&error, 0, "s?#", "test", 4)) + fail("json_pack failed to catch invalid format 's?#'"); + check_error(json_error_invalid_format, "Cannot use '#' on optional strings", "", 1, 2, 2); + /* string and length (int) */ value = json_pack("s#", "test asdf", 4); if(!json_is_string(value) || strcmp("test", json_string_value(value))) @@ -247,14 +257,32 @@ static void run_tests() value = json_pack("{s:s,s:o,s:O}", "a", NULL, "b", NULL, "c", NULL); if(value) fail("json_pack object optional incorrectly succeeded"); + value = json_pack("{s:**}", "a", NULL); if(value) fail("json_pack object optional invalid incorrectly succeeded"); + + if (json_pack_ex(&error, 0, "{s:i*}", "a", 1)) + fail("json_pack object optional invalid incorrectly succeeded"); + check_error(json_error_invalid_format, "Expected format 's', got '*'", "", 1, 5, 5); + value = json_pack("{s:s*,s:o*,s:O*}", "a", NULL, "b", NULL, "c", NULL); if(!json_is_object(value) || json_object_size(value) != 0) fail("json_pack object optional failed"); json_decref(value); + value = json_pack("{s:s*}", "key", "\xff\xff"); + if(value) + fail("json_pack object optional with invalid UTF-8 incorrectly succeeded"); + + if(json_pack_ex(&error, 0, "{s: s*#}", "key", "test", 1)) + fail("json_pack failed to catch invalid format 's*#'"); + check_error(json_error_invalid_format, "Cannot use '#' on optional strings", "", 1, 6, 6); + + if(json_pack_ex(&error, 0, "{s: s*+}", "key", "test", "ing")) + fail("json_pack failed to catch invalid format 's*+'"); + check_error(json_error_invalid_format, "Cannot use '+' on optional strings", "", 1, 6, 6); + /* simple array */ value = json_pack("[i,i,i]", 0, 1, 2); if(!json_is_array(value) || json_array_size(value) != 3) @@ -272,6 +300,11 @@ static void run_tests() value = json_pack("[s,o,O]", NULL, NULL, NULL); if(value) fail("json_pack array optional incorrectly succeeded"); + + if (json_pack_ex(&error, 0, "[i*]", 1)) + fail("json_pack array optional invalid incorrectly succeeded"); + check_error(json_error_invalid_format, "Unexpected format character '*'", "", 1, 3, 3); + value = json_pack("[**]", NULL); if(value) fail("json_pack array optional invalid incorrectly succeeded"); @@ -338,7 +371,7 @@ static void run_tests() /* NULL string */ if(json_pack_ex(&error, 0, "s", NULL)) fail("json_pack failed to catch null argument string"); - check_error(json_error_null_value, "NULL string argument", "", 1, 1, 1); + check_error(json_error_null_value, "NULL string", "", 1, 1, 1); /* + on its own */ if(json_pack_ex(&error, 0, "+", NULL)) @@ -353,13 +386,13 @@ static void run_tests() /* NULL key */ if(json_pack_ex(&error, 0, "{s:i}", NULL, 1)) fail("json_pack failed to catch NULL key"); - check_error(json_error_null_value, "NULL string argument", "", 1, 2, 2); + check_error(json_error_null_value, "NULL object key", "", 1, 2, 2); /* NULL value followed by object still steals the object's ref */ value = json_incref(json_object()); if(json_pack_ex(&error, 0, "{s:s,s:o}", "badnull", NULL, "dontleak", value)) fail("json_pack failed to catch NULL value"); - check_error(json_error_null_value, "NULL string argument", "", 1, 4, 4); + check_error(json_error_null_value, "NULL string", "", 1, 4, 4); if(value->refcount != (size_t)1) fail("json_pack failed to steal reference after error."); json_decref(value); @@ -389,6 +422,16 @@ static void run_tests() fail("json_pack failed to catch invalid UTF-8 in a string"); check_error(json_error_invalid_utf8, "Invalid UTF-8 string", "", 1, 4, 4); + /* Invalid UTF-8 in an optional '?' string */ + if(json_pack_ex(&error, 0, "{s:s?}", "foo", "\xff\xff")) + fail("json_pack failed to catch invalid UTF-8 in an optional '?' string"); + check_error(json_error_invalid_utf8, "Invalid UTF-8 string", "", 1, 5, 5); + + /* Invalid UTF-8 in an optional '*' string */ + if(json_pack_ex(&error, 0, "{s:s*}", "foo", "\xff\xff")) + fail("json_pack failed to catch invalid UTF-8 in an optional '*' string"); + check_error(json_error_invalid_utf8, "Invalid UTF-8 string", "", 1, 5, 5); + /* Invalid UTF-8 in a concatenated key */ if(json_pack_ex(&error, 0, "{s+:i}", "\xff\xff", "concat", 42)) fail("json_pack failed to catch invalid UTF-8 in an object key"); From 8d659113d53d7ef60eae6a6e2c5b0ecfc89fc74b Mon Sep 17 00:00:00 2001 From: Corey Farrell Date: Tue, 25 Sep 2018 17:34:25 -0400 Subject: [PATCH 2/2] More work on json_pack error reporting. * Remove errant line-feed from pack_object error message. * Correct error message in pack_object_inter. * Create pack_integer / pack_real to get the correct error messages on failure when packing numeric values. * Add tests for packing NAN and infinity directly, in an array and as an object value. --- src/pack_unpack.c | 46 +++++++++++++++++++++++++++---- test/suites/api/test_pack.c | 54 +++++++++++++++++++++++++++++++++++-- 2 files changed, 93 insertions(+), 7 deletions(-) diff --git a/src/pack_unpack.c b/src/pack_unpack.c index fc98df4..ec04bc3 100644 --- a/src/pack_unpack.c +++ b/src/pack_unpack.c @@ -261,7 +261,7 @@ static json_t *pack_object(scanner_t *s, va_list *ap) jsonp_free(key); if(valueOptional != '*') { - set_error(s, "", json_error_null_value, "NULL object value\n"); + set_error(s, "", json_error_null_value, "NULL object value"); s->has_error = 1; } @@ -396,11 +396,47 @@ static json_t *pack_object_inter(scanner_t *s, va_list *ap, int need_incref) break; } - set_error(s, "", json_error_null_value, "NULL object key"); + set_error(s, "", json_error_null_value, "NULL object"); s->has_error = 1; return NULL; } +static json_t *pack_integer(scanner_t *s, json_int_t value) +{ + json_t *json = json_integer(value); + + if (!json) { + set_error(s, "", json_error_out_of_memory, "Out of memory"); + s->has_error = 1; + } + + return json; +} + +static json_t *pack_real(scanner_t *s, double value) +{ + /* Allocate without setting value so we can identify OOM error. */ + json_t *json = json_real(0.0); + + if (!json) { + set_error(s, "", json_error_out_of_memory, "Out of memory"); + s->has_error = 1; + + return NULL; + } + + if (json_real_set(json, value)) { + json_decref(json); + + set_error(s, "", json_error_numeric_overflow, "Invalid floating point value"); + s->has_error = 1; + + return NULL; + } + + return json; +} + static json_t *pack(scanner_t *s, va_list *ap) { switch(token(s)) { @@ -420,13 +456,13 @@ static json_t *pack(scanner_t *s, va_list *ap) return va_arg(*ap, int) ? json_true() : json_false(); case 'i': /* integer from int */ - return json_integer(va_arg(*ap, int)); + return pack_integer(s, va_arg(*ap, int)); case 'I': /* integer from json_int_t */ - return json_integer(va_arg(*ap, json_int_t)); + return pack_integer(s, va_arg(*ap, json_int_t)); case 'f': /* real */ - return json_real(va_arg(*ap, double)); + return pack_real(s, va_arg(*ap, double)); case 'O': /* a json_t object; increments refcount */ return pack_object_inter(s, ap, 1); diff --git a/test/suites/api/test_pack.c b/test/suites/api/test_pack.c index 7e1c0b5..ab3aa91 100644 --- a/test/suites/api/test_pack.c +++ b/test/suites/api/test_pack.c @@ -15,8 +15,39 @@ #include #include #include +#include #include "util.h" +#ifdef INFINITY +// This test triggers "warning C4756: overflow in constant arithmetic" +// in Visual Studio. This warning is triggered here by design, so disable it. +// (This can only be done on function level so we keep these tests separate) +#ifdef _MSC_VER +#pragma warning(push) +#pragma warning (disable: 4756) +#endif +static void test_inifity() +{ + json_error_t error; + + if (json_pack_ex(&error, 0, "f", INFINITY)) + fail("json_pack infinity incorrectly succeeded"); + check_error(json_error_numeric_overflow, "Invalid floating point value", "", 1, 1, 1); + + if (json_pack_ex(&error, 0, "[f]", INFINITY)) + fail("json_pack infinity array element incorrectly succeeded"); + check_error(json_error_numeric_overflow, "Invalid floating point value", "", 1, 2, 2); + + if (json_pack_ex(&error, 0, "{s:f}", "key", INFINITY)) + fail("json_pack infinity object value incorrectly succeeded"); + check_error(json_error_numeric_overflow, "Invalid floating point value", "", 1, 4, 4); + +#ifdef _MSC_VER +#pragma warning(pop) +#endif +} +#endif // INFINITY + static void run_tests() { json_t *value; @@ -313,6 +344,25 @@ static void run_tests() fail("json_pack array optional failed"); json_decref(value); +#ifdef NAN + /* Invalid float values */ + if (json_pack_ex(&error, 0, "f", NAN)) + fail("json_pack NAN incorrectly succeeded"); + check_error(json_error_numeric_overflow, "Invalid floating point value", "", 1, 1, 1); + + if (json_pack_ex(&error, 0, "[f]", NAN)) + fail("json_pack NAN array element incorrectly succeeded"); + check_error(json_error_numeric_overflow, "Invalid floating point value", "", 1, 2, 2); + + if (json_pack_ex(&error, 0, "{s:f}", "key", NAN)) + fail("json_pack NAN object value incorrectly succeeded"); + check_error(json_error_numeric_overflow, "Invalid floating point value", "", 1, 4, 4); +#endif + +#ifdef INFINITY + test_inifity(); +#endif + /* Whitespace; regular string */ value = json_pack(" s\t ", "test"); if(!json_is_string(value) || strcmp("test", json_string_value(value))) @@ -439,9 +489,9 @@ static void run_tests() if(json_pack_ex(&error, 0, "{s:o}", "foo", NULL)) fail("json_pack failed to catch nullable object"); - check_error(json_error_null_value, "NULL object key", "", 1, 4, 4); + check_error(json_error_null_value, "NULL object", "", 1, 4, 4); if(json_pack_ex(&error, 0, "{s:O}", "foo", NULL)) fail("json_pack failed to catch nullable incref object"); - check_error(json_error_null_value, "NULL object key", "", 1, 4, 4); + check_error(json_error_null_value, "NULL object", "", 1, 4, 4); }