Merge pull request #924 from json-c/put_no_recurse

avoid recursion during json_object_put

CMakeLists.txt

@@ -267,6 +267,7 @@ if (HAVE_SYS_RANDOM_H)
 endif()
 if (HAVE_SYS_RESOURCE_H)
     check_symbol_exists(getrusage   "sys/resource.h" HAVE_GETRUSAGE)
+    check_symbol_exists(setrlimit   "sys/resource.h" HAVE_SETRLIMIT)
 endif()
 
 check_symbol_exists(strtoll     "stdlib.h" HAVE_STRTOLL)

arraylist.c

@@ -172,6 +172,14 @@ int array_list_put_idx(struct array_list *arr, size_t idx, void *data)
 	return 0;
 }
 
+int array_list_set_idx(struct array_list *arr, size_t idx, void *data)
+{
+	if (idx >= arr->length)
+		return -1;
+	arr->array[idx] = data;
+	return 0;
+}
+
 int array_list_add(struct array_list *arr, void *data)
 {
 	/* Repeat some of array_list_put_idx() so we can skip several

arraylist.h

@@ -68,6 +68,12 @@ extern int array_list_put_idx(struct array_list *al, size_t i, void *data);
 
 extern int array_list_add(struct array_list *al, void *data);
 
+/**
+ * Set the value at index i.  Caller is responsible for freeing the previous value.
+ * To automatically free the existing value, use array_list_put_idx() instead.
+ */
+extern int array_list_set_idx(struct array_list *al, size_t i, void *data);
+
 extern size_t array_list_length(struct array_list *al);
 
 extern void array_list_sort(struct array_list *arr, int (*compar)(const void *, const void *));

json-c.sym

@@ -17,6 +17,7 @@ JSONC_PRIVATE {
     array_list_free;
     array_list_new;
     array_list_put_idx;
+    array_list_set_idx;
     array_list_sort;
     json_hex_chars;
     json_parse_double;
@@ -24,6 +25,7 @@ JSONC_PRIVATE {
     json_parse_uint64;
     lh_table_delete;
     lh_table_delete_entry;
+    lh_table_delete_entry_to_tail;
     lh_table_free;
     lh_table_insert;
     lh_table_insert_w_hash;

json_object.c

@@ -269,11 +269,15 @@ struct json_object *json_object_get(struct json_object *jso)
 	return jso;
 }
 
-int json_object_put(struct json_object *jso)
-{
-	if (!jso)
-		return 0;
 
+/**
+  * Internal json_object_put function
+  * Returns 0 if we're done "freeing" the object, either because its memory
+  * was actually released, or we just needed to decrement the refcount.
+  * Returns 1 when the object is a non-empty container that still needs to be handled.
+  */
+static inline int _json_object_put_maybe_free(struct json_object *jso, int free_containers)
+{
 	/* Avoid invalid free and crash explicitly instead of (silently)
 	 * segfaulting.
 	 */
@@ -287,21 +291,157 @@ int json_object_put(struct json_object *jso)
 	 * operating on an already-freed object.
 	 */
 	if (__sync_sub_and_fetch(&jso->_ref_count, 1) > 0)
-		return 0;
 #else
 	if (--jso->_ref_count > 0)
-		return 0;
 #endif
+	{
+		return 0;  // All done, caller doesn't need to do anything else
+	}
 
 	if (jso->_user_delete)
 		jso->_user_delete(jso, jso->_userdata);
+
 	switch (jso->o_type)
 	{
-	case json_type_object: json_object_object_delete(jso); break;
-	case json_type_array: json_object_array_delete(jso); break;
-	case json_type_string: json_object_string_delete(jso); break;
-	default: json_object_generic_delete(jso); break;
+	case json_type_object: 
+		if (free_containers || lh_table_length(JC_OBJECT(jso)->c_object) == 0)
+		{
+			json_object_object_delete(jso);
+			break;
+		}
+		return 1;
+	case json_type_array: 
+		// container objects are handled by the caller
+		if (free_containers || array_list_length(JC_ARRAY(jso)->c_array) == 0)
+		{
+			json_object_array_delete(jso);
+			break;
+		}
+		return 1;
+	case json_type_string:
+		json_object_string_delete(jso);
+		break;
+	default:
+		json_object_generic_delete(jso);
+		break;
 	}
+	return 0;  // All done, caller doesn't need to do anything else
+}
+
+int json_object_put(struct json_object *jso)
+{
+	if (!jso)
+		return 0;
+
+	if (_json_object_put_maybe_free(jso, 0) == 0)
+		return 0;
+	// else, it's a non-empty container object, handle it below
+
+	// Note: jso is now a "zombie" object, _ref_count == 0 but memory not yet released
+
+	/*
+	 * Handle container objects with minimal stack usage.
+	 * Perform depth-first iteration, decrementing ref counts on way down
+	 * and freeing actual memory on the way up.
+	 * Iterate backwards through each container so we can use the tail
+	 * pointer/array length to know where to pick up upon popping up to
+	 * the parent.
+	 */
+
+	while(jso != NULL)
+	{
+		size_t total_slots;
+		size_t slots_left;
+		struct lh_entry *cur_entry = NULL;
+		int retry_main_loop = 0;
+
+		if (jso->o_type == json_type_object)
+		{
+			total_slots = lh_table_length(JC_OBJECT(jso)->c_object);
+			cur_entry = JC_OBJECT(jso)->c_object->tail;
+		}
+		else
+		{
+			total_slots = array_list_length(JC_ARRAY(jso)->c_array);
+		}
+		slots_left = total_slots;
+
+		while (slots_left > 0)
+		{
+			size_t cur_slot = slots_left - 1;
+			json_object *child;
+
+			// First, clear the slot in the current jso object
+			// The slot itself will be freed when jso is freed, or
+			// if the child object in the slot is a container too and
+			// and we "recurse" into it.
+			switch (jso->o_type)
+			{
+			case json_type_object: 
+				child = (json_object *)lh_entry_v(cur_entry);
+				// We're going to free child, so detach it from the entry
+				lh_entry_set_val(cur_entry, NULL);
+				break;
+			case json_type_array:
+				child = (struct json_object *)array_list_get_idx(JC_ARRAY(jso)->c_array, cur_slot);
+				// We're going to free child, so detach it from the entry
+				array_list_set_idx(JC_ARRAY(jso)->c_array, cur_slot, NULL);
+				break;
+			default:
+				assert(!"jso->o_type is not object or array");
+				break;
+			}
+
+			// Now, handle actually freeing the json_object in that slot
+			if (!child || _json_object_put_maybe_free(child, 0) == 0)
+			{
+ 				// child is either freed, or still referenced somewhere else
+				// leave it as-is and handle the previous slot
+				slots_left--;
+				if (jso->o_type == json_type_object)
+					cur_entry = cur_entry->prev;
+				continue;
+			}
+			// _ref_count == 0 now, and _user_delete has been called so we can re-use _userdata 
+			child->_delete_parent = jso;  // aka _userdata
+			child->_user_delete = NULL;   // make sure it's not called again
+
+			// Clear the slot entries whose json_object have been freed so when we pop
+			// back up to this jso we can continue where we left off.
+			// Note: since we set each entry to NULL above, clearing the slot
+			//  is a noop wrt releasing a json_object.
+			if (jso->o_type == json_type_object)
+			{
+				lh_table_delete_entry_to_tail(JC_OBJECT(jso)->c_object, cur_entry);
+			}
+			else // json_type_array
+			{
+				array_list_del_idx(JC_ARRAY(jso)->c_array, cur_slot, total_slots - cur_slot);
+			}
+			// Iterate down through the child, it will be freed once all 
+			// of *its* children are freed
+			jso = child;
+			retry_main_loop = 1;
+			break;
+		}
+
+		if (retry_main_loop)
+			// Iterating down, don't free jso yet
+			continue;
+
+		// All slots are cleared, now pop back up to the parent
+		{
+			json_object *parent = jso->_delete_parent;
+			// jso is a child that's already been detached from its parent
+			// so we need to actually free it now
+			assert(jso->_ref_count == 0);
+			jso->_ref_count++;   // We're the exclusive owner of jso, non-atomic add is ok.
+			assert(_json_object_put_maybe_free(jso, 1) == 0);
+			jso = parent;
+			// iteration will be reset at the top of the loop
+		}
+	}
+
 	return 1;
 }
 
@@ -516,9 +656,11 @@ static int json_object_object_to_json_string(struct json_object *jso, struct pri
 
 static void json_object_lh_entry_free(struct lh_entry *ent)
 {
+	struct json_object *jso = (struct json_object *)lh_entry_v(ent);
 	if (!lh_entry_k_is_constant(ent))
 		free(lh_entry_k(ent));
-	json_object_put((struct json_object *)lh_entry_v(ent));
+	if (jso) // micro-opt, skip func call on null object
+		json_object_put(jso);
 }
 
 static void json_object_object_delete(struct json_object *jso_base)
@@ -1497,7 +1639,9 @@ static int json_object_array_to_json_string(struct json_object *jso, struct prin
 
 static void json_object_array_entry_free(void *data)
 {
-	json_object_put((struct json_object *)data);
+	struct json_object *jso = (struct json_object *)data;
+	if (jso) // micro-opt, skip func call on null object
+		json_object_put(jso);
 }
 
 static void json_object_array_delete(struct json_object *jso)

json_object_private.h

@@ -47,7 +47,10 @@ struct json_object
 	json_object_to_json_string_fn *_to_json_string;
 	struct printbuf *_pb;
 	json_object_delete_fn *_user_delete;
-	void *_userdata;
+	union {
+		void *_userdata;
+		void *_delete_parent;  // Used during json_object_put
+	};
 	// Actually longer, always malloc'd as some more-specific type.
 	// The rest of a struct json_object_${o_type} follows
 };

linkhash.c

@@ -668,6 +668,8 @@ int lh_table_delete_entry(struct lh_table *t, struct lh_entry *e)
 	/* CAW: fixed to be 64bit nice, still need the crazy negative case... */
 	ptrdiff_t n = (ptrdiff_t)(e - t->table);
 
+	assert(n >= 0 && n < t->size);
+
 	/* CAW: this is bad, really bad, maybe stack goes other direction on this machine... */
 	if (n < 0)
 	{
@@ -704,6 +706,22 @@ int lh_table_delete_entry(struct lh_table *t, struct lh_entry *e)
 	return 0;
 }
 
+int lh_table_delete_entry_to_tail(struct lh_table *t, struct lh_entry *first_entry)
+{
+	struct lh_entry *del_entry = t->tail;
+	do
+	{
+		struct lh_entry *prev = del_entry->prev;
+		// Could probably micro-optimize this, but better to avoid code duplication for now
+		if (lh_table_delete_entry(t, del_entry) != 0)
+			return -1;
+		if (del_entry == first_entry)
+			break;
+		del_entry = prev;
+	} while (del_entry != NULL);
+	return 0;
+}
+
 int lh_table_delete(struct lh_table *t, const void *k)
 {
 	struct lh_entry *e = lh_table_lookup_entry(t, k);

linkhash.h

@@ -303,6 +303,17 @@ extern json_bool lh_table_lookup_ex(struct lh_table *t, const void *k, void **v)
  */
 extern int lh_table_delete_entry(struct lh_table *t, struct lh_entry *e);
 
+/**
+ * Delete all entries from the specified one to the tail of the list.
+ * Same as calling lh_table_delete_entry() on each of them.
+ *
+ * @param t the table to delete from.
+ * @param e a pointer to the first entry to delete.
+ * @return 0 if the item was deleted.
+ * @return -1 if it was not found.
+ */
+extern int lh_table_delete_entry_to_tail(struct lh_table *t, struct lh_entry *e);
+
 /**
  * Delete a record from the table.
  *

tests/CMakeLists.txt

@@ -21,6 +21,7 @@ set(ALL_TEST_NAMES
     test_charcase
     test_compare
     test_deep_copy
+    test_deep_nesting
     test_double_serializer
     test_float
     test_int_add

tests/test_deep_nesting.c

@@ -0,0 +1,84 @@
+#ifdef NDEBUG
+#undef NDEBUG
+#endif
+#include <limits.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#ifdef HAVE_SYS_RESOURCE_H
+#include <sys/resource.h>
+#endif
+
+#include "config.h"
+
+#include "json.h"
+
+#define NESTING_DEPTH 100000
+
+static char *generate_json_string(void);
+static char *generate_json_string()
+{
+	char *str;
+	int depth = NESTING_DEPTH;
+	str = malloc(depth * 2 + 1);
+	memset(str, '[', depth);
+	memset(str + depth, ']', depth);
+	str[depth * 2] = '\0';
+	return str;
+}
+
+static void test_deep_nesting_put(const char *str);
+static void test_deep_nesting_put(const char *str)
+{
+	json_object *my_array;
+
+	struct json_tokener *tok = json_tokener_new_ex(NESTING_DEPTH);
+	my_array = json_tokener_parse_ex(tok, str, strlen(str) + 1);
+	printf("Parsed depth %d string to json_object: %s\n", NESTING_DEPTH, (my_array == NULL) ? "NO" : "yes");
+
+	json_object_put(my_array);
+	printf("Freed json_object\n");
+
+	json_tokener_free(tok);
+}
+
+static void test_deep_nesting_tostring(const char *str);
+static void test_deep_nesting_tostring(const char *str)
+{
+	json_object *my_array;
+
+	struct json_tokener *tok = json_tokener_new_ex(NESTING_DEPTH);
+	my_array = json_tokener_parse_ex(tok, str, strlen(str) + 1);
+	printf("Parsed depth %d string to json_object: %s\n", NESTING_DEPTH, (my_array == NULL) ? "NO" : "yes");
+
+	const char *res = json_object_to_json_string_ext(my_array, JSON_C_TO_STRING_PLAIN);
+	printf("Serialized to string of length %ld\n", (long)strlen(res));
+	json_object_put(my_array);
+	printf("Freed json_object\n");
+
+	json_tokener_free(tok);
+}
+
+int main(int argc, char **argv)
+{
+	char *str;	
+#ifdef HAVE_SETRLIMIT
+	struct rlimit rl;
+	rl.rlim_cur = 2048;
+	rl.rlim_max = 2048;
+	setrlimit(RLIMIT_STACK, &rl);
+#endif
+
+	str = generate_json_string();
+
+	MC_SET_DEBUG(1);
+
+	test_deep_nesting_put(str);
+
+	if (0) // TODO: make json_object_to_json_string non-recursive
+		test_deep_nesting_tostring(str);
+
+	free(str);
+
+	return EXIT_SUCCESS;
+}

tests/test_deep_nesting.expected

@@ -0,0 +1,2 @@
+Parsed depth 1000000 string to json_object: yes
+Freed json_object

tests/test_deep_nesting.test

@@ -0,0 +1 @@
+test_basic.test