Merge pull request #374 from coreyfarrell/always-steal

json_pack: Enable more complete stealing of references.

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

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);
+            s->has_error = 1;
-            return NULL;
         }
 
         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);
+            s->has_error = 1;
-            return NULL;
         }
 
         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)
+        if (!key)
-            goto error;
+            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);
+            } else {
-                continue;
+                s->has_error = 1;
             }
 
-            goto error;
+            next_token(s);
+            continue;
         }
 
-        if(json_object_set_new_nocheck(object, key, value)) {
+        if(s->has_error)
-            set_error(s, "<internal>", json_error_out_of_memory, "Unable to add key \"%s\"", key);
+            json_decref(value);
-            if(ours)
-                jsonp_free(key);
 
-            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);
+            } else {
-                continue;
+                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;
 }

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;

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");