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

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, "<args>", 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, "<internal>", 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, "<args>", 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, "<internal>", 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, "<internal>", 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, "<format>", 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, "<internal>", 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, "<format>", 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, "<format>", 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", "<args>", 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", "<args>", 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");