Fix leaks and crashes on allocation failure

Found by failing each allocation in turn under ASan/UBSan/LSan:

- plist_free() leaked the whole tree if its work stack could not be
  allocated; free the children iteratively without extra memory
- plist_copy(): when a shallow copy of a dict/array failed, the original's
  lookup table was freed along with the copy's data (use-after-free)
- bplist parser: fix a NULL dereference in parse_date_node(), check the
  node_attach() results, and don't leak on failure; plist_to_bin() no
  longer leaks on an allocation failure
- free the plist data when node creation fails (plist constructors, XML,
  JSON and binary parsers)

4 files changed, 86 insertions, 82 deletions

diff --git a/src/bplist.c b/src/bplist.c
index 45caae4..74348b8 100644
--- a/src/bplist.c
+++ b/src/bplist.c
@@ -277,6 +277,16 @@ void plist_bin_set_debug(int debug)
 
 static plist_t parse_bin_node_at_index(struct bplist_data *bplist, uint32_t node_index);
 
+static plist_t bplist_node_new(plist_data_t data)
+{
+    plist_t node = node_create(NULL, data);
+    if (!node) {
+        PLIST_BIN_ERR("%s: failed to create node\n", __func__);
+        plist_free_data(data);
+    }
+    return node;
+}
+
 static plist_t parse_int_node(const char **bnode, uint8_t size)
 {
     plist_data_t data = plist_new_plist_data();
@@ -308,7 +318,7 @@ static plist_t parse_int_node(const char **bnode, uint8_t size)
     (*bnode) += size;
     data->type = PLIST_INT;
 
-    return node_create(NULL, data);
+    return bplist_node_new(data);
 }
 
 static plist_t parse_real_node(const char **bnode, uint8_t size)
@@ -350,12 +360,15 @@ static plist_t parse_real_node(const char **bnode, uint8_t size)
     data->type = PLIST_REAL;
     data->length = sizeof(double);
 
-    return node_create(NULL, data);
+    return bplist_node_new(data);
 }
 
 static plist_t parse_date_node(const char **bnode, uint8_t size)
 {
     plist_t node = parse_real_node(bnode, size);
+    if (!node) {
+        return NULL;
+    }
     plist_data_t data = plist_get_data(node);
 
     data->type = PLIST_DATE;
@@ -382,7 +395,7 @@ static plist_t parse_string_node(const char **bnode, uint64_t size)
     data->strval[size] = '\0';
     data->length = strlen(data->strval);
 
-    return node_create(NULL, data);
+    return bplist_node_new(data);
 }
 
 static char *plist_utf16be_to_utf8(uint16_t *unistr, size_t len, size_t *items_read, size_t *items_written)
@@ -478,7 +491,7 @@ static plist_t parse_unicode_node(const char **bnode, uint64_t size)
     }
     data->length = items_written;
 
-    return node_create(NULL, data);
+    return bplist_node_new(data);
 }
 
 static plist_t parse_data_node(const char **bnode, uint64_t size)
@@ -498,7 +511,7 @@ static plist_t parse_data_node(const char **bnode, uint64_t size)
     }
     memcpy(data->buff, *bnode, sizeof(uint8_t) * size);
 
-    return node_create(NULL, data);
+    return bplist_node_new(data);
 }
 
 static plist_t parse_dict_node(struct bplist_data *bplist, const char** bnode, uint64_t size)
@@ -582,8 +595,17 @@ static plist_t parse_dict_node(struct bplist_data *bplist, const char** bnode, u
             return NULL;
         }
 
-        node_attach((node_t)node, (node_t)key);
-        node_attach((node_t)node, (node_t)val);
+        if (node_attach((node_t)node, (node_t)key) != NODE_ERR_SUCCESS) {
+            plist_free(key);
+            plist_free(val);
+            plist_free(node);
+            return NULL;
+        }
+        if (node_attach((node_t)node, (node_t)val) != NODE_ERR_SUCCESS) {
+            plist_free(val);
+            plist_free(node);
+            return NULL;
+        }
     }
 
     return node;
@@ -636,7 +658,11 @@ static plist_t parse_array_node(struct bplist_data *bplist, const char** bnode,
             return NULL;
         }
 
-        node_attach((node_t)node, (node_t)val);
+        if (node_attach((node_t)node, (node_t)val) != NODE_ERR_SUCCESS) {
+            plist_free(val);
+            plist_free(node);
+            return NULL;
+        }
     }
 
     return node;
@@ -661,7 +687,7 @@ static plist_t parse_uid_node(const char **bnode, uint8_t size)
     data->type = PLIST_UID;
     data->length = sizeof(uint64_t);
 
-    return node_create(NULL, data);
+    return bplist_node_new(data);
 }
 
 static plist_t parse_bin_node(struct bplist_data *bplist, const char** object)
@@ -726,7 +752,7 @@ static plist_t parse_bin_node(struct bplist_data *bplist, const char** object)
             data->type = PLIST_BOOLEAN;
             data->boolval = TRUE;
             data->length = 1;
-            return node_create(NULL, data);
+            return bplist_node_new(data);
         }
 
         case BPLIST_FALSE:
@@ -739,7 +765,7 @@ static plist_t parse_bin_node(struct bplist_data *bplist, const char** object)
             data->type = PLIST_BOOLEAN;
             data->boolval = FALSE;
             data->length = 1;
-            return node_create(NULL, data);
+            return bplist_node_new(data);
         }
 
         case BPLIST_NULL:
@@ -751,7 +777,7 @@ static plist_t parse_bin_node(struct bplist_data *bplist, const char** object)
             }
             data->type = PLIST_NULL;
             data->length = 0;
-            return node_create(NULL, data);
+            return bplist_node_new(data);
         }
 
         default:
@@ -1550,6 +1576,9 @@ plist_err_t plist_to_bin(plist_t plist, char **plist_bin, uint32_t * length)
     //write objects and table
     offsets = (uint64_t *) malloc(num_objects * sizeof(uint64_t));
     if (!offsets) {
+        byte_array_free(bplist_buff);
+        ptr_array_free(objects);
+        hash_table_destroy(ref_table);
         return PLIST_ERR_NO_MEM;
     }
     for (i = 0; i < num_objects; i++)
diff --git a/src/jplist.c b/src/jplist.c
index fe0b5cb..193d231 100644
--- a/src/jplist.c
+++ b/src/jplist.c
@@ -581,6 +581,9 @@ static plist_t parse_primitive(const char* js, jsmntok_info_t* ti, int* index)
         }
         data->type = PLIST_NULL;
         val = plist_new_node(data);
+        if (!val) {
+            plist_free_data(data);
+        }
     } else if (isdigit(str_val[0]) || (str_val[0] == '-' && str_val+1 < str_end && isdigit(str_val[1]))) {
         char* endp = (char*)str_val;
         int is_neg = (str_val[0] == '-');
diff --git a/src/plist.c b/src/plist.c
index 8d3c8ef..677e94e 100644
--- a/src/plist.c
+++ b/src/plist.c
@@ -349,6 +349,16 @@ plist_t plist_new_node(plist_data_t data)
     return (plist_t) node_create(NULL, data);
 }
 
+/* like plist_new_node(), but takes care of data if the node can't be created */
+static plist_t plist_new_node_or_free(plist_data_t data)
+{
+    plist_t node = plist_new_node(data);
+    if (!node) {
+        plist_free_data(data);
+    }
+    return node;
+}
+
 plist_data_t plist_get_data(plist_t node)
 {
     if (!node)
@@ -428,71 +438,30 @@ static int plist_free_children(node_t root)
 {
     if (!root) return NODE_ERR_INVALID_ARG;
 
-    if (!node_first_child(root)) {
-        return NODE_ERR_SUCCESS;
-    }
-
-    size_t cap = 64, sp = 0;
-    node_t *stack = (node_t*)malloc(cap * sizeof(*stack));
-    if (!stack) return NODE_ERR_NO_MEM;
-
-    // Push *direct* children onto the stack, detached from root.
+    /* Walk down to the deepest first child, free it and step back up.
+     * This needs no extra memory, so freeing can't fail with out-of-memory
+     * (which would leak the whole tree). */
+    node_t cur = root;
     for (;;) {
-        node_t ch = node_first_child(root);
-        if (!ch) break;
-
-        int di = node_detach(root, ch);
-        if (di < 0) {
-            free(stack);
-            return di;
-        }
-
-        if (sp == cap) {
-            cap += 64;
-            node_t *tmp = (node_t*)realloc(stack, cap * sizeof(*stack));
-            if (!tmp) {
-                free(stack);
-                return NODE_ERR_NO_MEM;
-            }
-            stack = tmp;
-        }
-        stack[sp++] = ch;
-    }
-
-    // Now free the detached subtree nodes (and their descendants).
-    while (sp) {
-        node_t node = stack[sp - 1];
-        node_t ch = node_first_child(node);
+        node_t ch = node_first_child(cur);
         if (ch) {
-            int di = node_detach(node, ch);
-            if (di < 0) {
-                free(stack);
-                return di;
-            }
-
-            if (sp == cap) {
-                cap += 64;
-                node_t *tmp = (node_t*)realloc(stack, cap * sizeof(*stack));
-                if (!tmp) {
-                    free(stack);
-                    return NODE_ERR_NO_MEM;
-                }
-                stack = tmp;
-            }
-            stack[sp++] = ch;
+            cur = ch;
             continue;
         }
+        if (cur == root) break;
 
-        plist_data_t data = plist_get_data(node);
+        node_t parent = (node_t)cur->parent;
+        int di = node_detach(parent, cur);
+        if (di < 0) {
+            return di;
+        }
+        plist_data_t data = plist_get_data(cur);
         plist_free_data(data);
-        node->data = NULL;
-
-        node_destroy(node);
-
-        sp--;
+        cur->data = NULL;
+        node_destroy(cur);
+        cur = parent;
     }
 
-    free(stack);
     return NODE_ERR_SUCCESS;
 }
 
@@ -532,7 +501,7 @@ plist_t plist_new_dict(void)
         return NULL;
     }
     data->type = PLIST_DICT;
-    return plist_new_node(data);
+    return plist_new_node_or_free(data);
 }
 
 plist_t plist_new_array(void)
@@ -543,7 +512,7 @@ plist_t plist_new_array(void)
         return NULL;
     }
     data->type = PLIST_ARRAY;
-    return plist_new_node(data);
+    return plist_new_node_or_free(data);
 }
 
 //These nodes should not be handled by users
@@ -563,7 +532,7 @@ static plist_t plist_new_key(const char *val)
     } else {
         data->length = strlen(val);
     }
-    return plist_new_node(data);
+    return plist_new_node_or_free(data);
 }
 
 plist_t plist_new_string(const char *val)
@@ -582,7 +551,7 @@ plist_t plist_new_string(const char *val)
     } else {
         data->length = strlen(val);
     }
-    return plist_new_node(data);
+    return plist_new_node_or_free(data);
 }
 
 plist_t plist_new_bool(uint8_t val)
@@ -595,7 +564,7 @@ plist_t plist_new_bool(uint8_t val)
     data->type = PLIST_BOOLEAN;
     data->boolval = val;
     data->length = sizeof(uint8_t);
-    return plist_new_node(data);
+    return plist_new_node_or_free(data);
 }
 
 plist_t plist_new_uint(uint64_t val)
@@ -608,7 +577,7 @@ plist_t plist_new_uint(uint64_t val)
     data->type = PLIST_INT;
     data->intval = val;
     data->length = (val > INT_MAX) ? sizeof(uint64_t)*2 : sizeof(uint64_t);
-    return plist_new_node(data);
+    return plist_new_node_or_free(data);
 }
 
 plist_t plist_new_int(int64_t val)
@@ -621,7 +590,7 @@ plist_t plist_new_int(int64_t val)
     data->type = PLIST_INT;
     data->intval = val;
     data->length = sizeof(uint64_t);
-    return plist_new_node(data);
+    return plist_new_node_or_free(data);
 }
 
 plist_t plist_new_uid(uint64_t val)
@@ -634,7 +603,7 @@ plist_t plist_new_uid(uint64_t val)
     data->type = PLIST_UID;
     data->intval = val;
     data->length = sizeof(uint64_t);
-    return plist_new_node(data);
+    return plist_new_node_or_free(data);
 }
 
 plist_t plist_new_real(double val)
@@ -647,7 +616,7 @@ plist_t plist_new_real(double val)
     data->type = PLIST_REAL;
     data->realval = val;
     data->length = sizeof(double);
-    return plist_new_node(data);
+    return plist_new_node_or_free(data);
 }
 
 plist_t plist_new_data(const char *val, uint64_t length)
@@ -667,7 +636,7 @@ plist_t plist_new_data(const char *val, uint64_t length)
         memcpy(data->buff, val, length);
     }
     data->length = length;
-    return plist_new_node(data);
+    return plist_new_node_or_free(data);
 }
 
 plist_t plist_new_date(int32_t sec, int32_t usec)
@@ -680,7 +649,7 @@ plist_t plist_new_date(int32_t sec, int32_t usec)
     data->type = PLIST_DATE;
     data->realval = (double)sec + (double)usec / 1000000;
     data->length = sizeof(double);
-    return plist_new_node(data);
+    return plist_new_node_or_free(data);
 }
 
 plist_t plist_new_unix_date(int64_t sec)
@@ -693,7 +662,7 @@ plist_t plist_new_unix_date(int64_t sec)
     data->type = PLIST_DATE;
     data->realval = (double)sec - MAC_EPOCH;
     data->length = sizeof(double);
-    return plist_new_node(data);
+    return plist_new_node_or_free(data);
 }
 
 plist_t plist_new_null(void)
@@ -706,7 +675,7 @@ plist_t plist_new_null(void)
     data->type = PLIST_NULL;
     data->intval = 0;
     data->length = 0;
-    return plist_new_node(data);
+    return plist_new_node_or_free(data);
 }
 
 void plist_free(plist_t plist)
@@ -771,6 +740,7 @@ static int plist_copy_node_shallow(node_t node, plist_t *out_newnode, plist_data
             break;
 
         case PLIST_ARRAY:
+            newdata->hashtable = NULL; // don't share (or free) the original's table on error
             if (data->hashtable) {
                 ptrarray_t* pa = ptr_array_new(((ptrarray_t*)data->hashtable)->capacity);
                 if (!pa) {
@@ -782,6 +752,7 @@ static int plist_copy_node_shallow(node_t node, plist_t *out_newnode, plist_data
             break;
 
         case PLIST_DICT:
+            newdata->hashtable = NULL; // don't share (or free) the original's table on error
             if (data->hashtable) {
                 hashtable_t* ht = hash_table_new(dict_key_hash, dict_key_compare, NULL);
                 if (!ht) {
diff --git a/src/xplist.c b/src/xplist.c
index 19d98ed..adf3f40 100644
--- a/src/xplist.c
+++ b/src/xplist.c
@@ -1212,6 +1212,7 @@ static plist_err_t node_from_xml(parse_ctx ctx, plist_t *plist)
             }
             subnode = plist_new_node(data);
             if (!subnode) {
+                plist_free_data(data);
                 PLIST_XML_ERR("failed to create node\n");
                 ctx->err = PLIST_ERR_NO_MEM;
                 goto err_out;