add_item_to_object: Fix use-after-free when string is aliased

If the `string` property of the item that is added is an alias to the
`string` parameter of `add_item_to_object`, and `constant` is false,
`cJSON_strdup` would access the string after it has been freed.

Thanks @hhallen for reporting this in DaveGamble#248.

cJSON.c

@@ -1895,33 +1895,38 @@ static void* cast_away_const(const void* string)
 
 static cJSON_bool add_item_to_object(cJSON * const object, const char * const string, cJSON * const item, const internal_hooks * const hooks, const cJSON_bool constant_key)
 {
+    char *new_key = NULL;
+    int new_type = cJSON_Invalid;
+
     if ((object == NULL) || (string == NULL) || (item == NULL))
     {
         return false;
     }
 
-    if (!(item->type & cJSON_StringIsConst) && (item->string != NULL))
-    {
-        hooks->deallocate(item->string);
-    }
-
     if (constant_key)
     {
-        item->string = (char*)cast_away_const(string);
-        item->type |= cJSON_StringIsConst;
+        new_key = (char*)cast_away_const(string);
+        new_type = item->type | cJSON_StringIsConst;
     }
     else
     {
-        char *key = (char*)cJSON_strdup((const unsigned char*)string, hooks);
-        if (key == NULL)
+        new_key = (char*)cJSON_strdup((const unsigned char*)string, hooks);
+        if (new_key == NULL)
         {
             return false;
         }
 
-        item->string = key;
-        item->type &= ~cJSON_StringIsConst;
+        new_type = item->type & ~cJSON_StringIsConst;
     }
 
+    if (!(item->type & cJSON_StringIsConst) && (item->string != NULL))
+    {
+        hooks->deallocate(item->string);
+    }
+
+    item->string = new_key;
+    item->type = new_type;
+
     return add_item_to_array(object, item);
 }
 

tests/misc_tests.c

@@ -508,6 +508,25 @@ static void cjson_create_array_reference_should_create_an_array_reference(void)
     cJSON_Delete(number_reference);
 }
 
+static void cjson_add_item_to_object_should_not_use_after_free_when_string_is_aliased(void)
+{
+    cJSON *object = cJSON_CreateObject();
+    cJSON *number = cJSON_CreateNumber(42);
+    char *name = (char*)cJSON_strdup((const unsigned char*)"number", &global_hooks);
+
+    TEST_ASSERT_NOT_NULL(object);
+    TEST_ASSERT_NOT_NULL(number);
+    TEST_ASSERT_NOT_NULL(name);
+
+    number->string = name;
+
+    /* The following should not have a use after free
+     * that would show up in valgrind or with AddressSanitizer */
+    cJSON_AddItemToObject(object, number->string, number);
+
+    cJSON_Delete(object);
+}
+
 int main(void)
 {
     UNITY_BEGIN();
@@ -530,6 +549,7 @@ int main(void)
     RUN_TEST(cjson_create_string_reference_should_create_a_string_reference);
     RUN_TEST(cjson_create_object_reference_should_create_an_object_reference);
     RUN_TEST(cjson_create_array_reference_should_create_an_array_reference);
+    RUN_TEST(cjson_add_item_to_object_should_not_use_after_free_when_string_is_aliased);
 
     return UNITY_END();
 }