From 9a1d9c88fce268b812453d0d23237ee805b9bc34 Mon Sep 17 00:00:00 2001 From: Corey Farrell Date: Mon, 6 Nov 2017 22:48:50 -0500 Subject: [PATCH 1/3] json_pack: Enable more complete stealing of references. Users of the "o" format have an expectation that the object reference will be stolen. Any error causes the collection process to end early. This patch causes json_pack and related functions to continue scanning the format and parameters so all references can be stolen to prevent leaks. This makes no attempt to continue processing if the format string is broken or missing. 'make check' still passes. Ran test_pack under valgrind and verified that the leaked reference is fixed. Added a test which uses refcounts to verify that the reference was correctly stolen after a NULL value error. Issue #135 --- src/pack_unpack.c | 68 ++++++++++++++++++++++++------------- test/suites/api/test_pack.c | 9 +++++ 2 files changed, 53 insertions(+), 24 deletions(-) diff --git a/src/pack_unpack.c b/src/pack_unpack.c index 72425a4..153f64d 100644 --- a/src/pack_unpack.c +++ b/src/pack_unpack.c @@ -29,6 +29,7 @@ typedef struct { int line; int column; size_t pos; + int has_error; } scanner_t; #define token(scanner) ((scanner)->token.token) @@ -60,6 +61,7 @@ static void scanner_init(scanner_t *s, json_error_t *error, s->line = 1; s->column = 0; s->pos = 0; + s->has_error = 0; } static void next_token(scanner_t *s) @@ -136,6 +138,7 @@ static char *read_string(scanner_t *s, va_list *ap, t = token(s); prev_token(s); + *ours = 0; if(t != '#' && t != '%' && t != '+') { /* Optimize the simple case */ str = va_arg(*ap, const char *); @@ -153,7 +156,6 @@ static char *read_string(scanner_t *s, va_list *ap, } *out_len = length; - *ours = 0; return (char *)str; } @@ -163,8 +165,7 @@ 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"); - strbuffer_close(&strbuff); - return NULL; + s->has_error = 1; } next_token(s); @@ -177,13 +178,12 @@ static char *read_string(scanner_t *s, va_list *ap, } else { prev_token(s); - length = strlen(str); + length = s->has_error ? 0 : strlen(str); } - if(strbuffer_append_bytes(&strbuff, str, length) == -1) { + if(!s->has_error && strbuffer_append_bytes(&strbuff, str, length) == -1) { set_error(s, "", json_error_out_of_memory, "Out of memory"); - strbuffer_close(&strbuff); - return NULL; + s->has_error = 1; } next_token(s); @@ -193,9 +193,15 @@ static char *read_string(scanner_t *s, va_list *ap, } } + if(s->has_error) { + strbuffer_close(&strbuff); + return NULL; + } + if(!utf8_check_string(strbuff.value, strbuff.length)) { set_error(s, "", json_error_invalid_utf8, "Invalid UTF-8 %s", purpose); strbuffer_close(&strbuff); + s->has_error = 1; return NULL; } @@ -226,8 +232,8 @@ static json_t *pack_object(scanner_t *s, va_list *ap) } key = read_string(s, ap, "object key", &len, &ours); - if(!key) - goto error; + if (!key) + s->has_error = 1; next_token(s); @@ -238,19 +244,20 @@ static json_t *pack_object(scanner_t *s, va_list *ap) if(strchr("soO", token(s)) && s->next_token.token == '*') { next_token(s); - next_token(s); - continue; + } else { + s->has_error = 1; } - goto error; + next_token(s); + continue; } - if(json_object_set_new_nocheck(object, key, value)) { - set_error(s, "", json_error_out_of_memory, "Unable to add key \"%s\"", key); - if(ours) - jsonp_free(key); + if(s->has_error) + json_decref(value); - goto error; + if(!s->has_error && json_object_set_new_nocheck(object, key, value)) { + set_error(s, "", json_error_out_of_memory, "Unable to add key \"%s\"", key); + s->has_error = 1; } if(ours) @@ -261,7 +268,8 @@ static json_t *pack_object(scanner_t *s, va_list *ap) next_token(s); } - return object; + if(!s->has_error) + return object; error: json_decref(object); @@ -278,6 +286,7 @@ static json_t *pack_array(scanner_t *s, va_list *ap) if(!token(s)) { set_error(s, "", json_error_invalid_format, "Unexpected end of format string"); + /* Format string errors are unrecoverable. */ goto error; } @@ -285,23 +294,29 @@ static json_t *pack_array(scanner_t *s, va_list *ap) if(!value) { if(strchr("soO", token(s)) && s->next_token.token == '*') { next_token(s); - next_token(s); - continue; + } else { + s->has_error = 1; } - goto error; + next_token(s); + continue; } - if(json_array_append_new(array, value)) { + if(s->has_error) + json_decref(value); + + if(!s->has_error && json_array_append_new(array, value)) { set_error(s, "", json_error_out_of_memory, "Unable to append to array"); - goto error; + s->has_error = 1; } if(strchr("soO", token(s)) && s->next_token.token == '*') next_token(s); next_token(s); } - return array; + + if(!s->has_error) + return array; error: json_decref(array); @@ -396,6 +411,7 @@ static json_t *pack(scanner_t *s, va_list *ap) default: set_error(s, "", json_error_invalid_format, "Unexpected format character '%c'", token(s)); + s->has_error = 1; return NULL; } } @@ -798,6 +814,10 @@ json_t *json_vpack_ex(json_error_t *error, size_t flags, set_error(&s, "", json_error_invalid_format, "Garbage after format string"); return NULL; } + if(s.has_error) { + json_decref(value); + return NULL; + } return value; } diff --git a/test/suites/api/test_pack.c b/test/suites/api/test_pack.c index 02631a0..fa44b8b 100644 --- a/test/suites/api/test_pack.c +++ b/test/suites/api/test_pack.c @@ -352,6 +352,15 @@ static void run_tests() fail("json_pack failed to catch NULL key"); check_error(json_error_null_value, "NULL string argument", "", 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); + if(value->refcount != (size_t)1) + fail("json_pack failed to steal reference after error."); + json_decref(value); + /* More complicated checks for row/columns */ if(json_pack_ex(&error, 0, "{ {}: s }", "foo")) fail("json_pack failed to catch object as key"); From 89dad8959b7903b1ab3653d11b3bd4335c44015e Mon Sep 17 00:00:00 2001 From: Corey Farrell Date: Tue, 14 Nov 2017 22:44:48 -0500 Subject: [PATCH 2/3] json_object_iter_set_new: Fix error branch leak. This function needs to release a reference to value if the other arguments are invalid. Issue #135 --- src/value.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/value.c b/src/value.c index 86fda0a..94869f4 100644 --- a/src/value.c +++ b/src/value.c @@ -258,7 +258,10 @@ json_t *json_object_iter_value(void *iter) int json_object_iter_set_new(json_t *json, void *iter, json_t *value) { if(!json_is_object(json) || !iter || !value) + { + json_decref(value); return -1; + } hashtable_iter_set(iter, value); return 0; From 6c78910011c5d10b9fcb95b6fdd038f4aa9bf74d Mon Sep 17 00:00:00 2001 From: Corey Farrell Date: Tue, 14 Nov 2017 23:15:53 -0500 Subject: [PATCH 3/3] apiref: Clarify documentation for unpack `O` format. The `O` format causes reference counts to increase, but in an error they are not released. Callers to unpack functions that use the `O` format should use pointers pre-initialized to NULL so they can safely release the reference on error. Also corrected typo which said this was like `O` (itself). Fixes #135 --- doc/apiref.rst | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/doc/apiref.rst b/doc/apiref.rst index 7afeb21..a8cf8d6 100644 --- a/doc/apiref.rst +++ b/doc/apiref.rst @@ -1564,7 +1564,10 @@ type whose address should be passed. Store a JSON value with no conversion to a :type:`json_t` pointer. ``O`` (any value) [json_t \*] - Like ``O``, but the JSON value's reference count is incremented. + Like ``o``, but the JSON value's reference count is incremented. + Storage pointers should be initialized NULL before using unpack. + The caller is responsible for releasing all references incremented + by unpack, even when an error occurs. ``[fmt]`` (array) Convert each item in the JSON array according to the inner format