Issue #923: Avoid stack recursion in json_object_put() so heavily nested object trees...

diff --git a/arraylist.h b/arraylist.h
index 5b794100c6664f837c640ec7fbc08811baeabc37..84438ab55cdb8f8dc9edf0b9029f8d846e201323 100644 (file)
--- a/arraylist.h
+++ b/arraylist.h
@@ -72,7 +72,7 @@ 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 int array_list_set_idx(struct array_list *al, size_t i, void *data);
 
 extern size_t array_list_length(struct array_list *al);
 

diff --git a/json_object.c b/json_object.c
index d67a61a4b15820797e902b367a4c73c668c87feb..db79d6eb5d7c734853a4f2145cb68309e602929b 100644 (file)
--- a/json_object.c
+++ b/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)

diff --git a/json_object_private.h b/json_object_private.h
index f81af881f6a41dcaec1bc541e146b06f087d30c3..b96c74ba8183906a41f55537be26781e41512317 100644 (file)
--- a/json_object_private.h
+++ b/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
 };

diff --git a/linkhash.c b/linkhash.c
index 1856569650d2ca07a0773ce125b5464f62ad3576..04d1ff5d4b17d0597286e32fab2292b025a92e5c 100644 (file)
--- a/linkhash.c
+++ b/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)
        {
@@ -709,10 +711,14 @@ int lh_table_delete_entry_to_tail(struct lh_table *t, struct lh_entry *first_ent
        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;
-       } while (del_entry != first_entry);
+               if (del_entry == first_entry)
+                       break;
+               del_entry = prev;
+       } while (del_entry != NULL);
        return 0;
 }