aug_rm: fix segfault when deleting a tree and one of its ancestors

In a tree like /files/1/2, when we execute 'rm /files//*', the path
expression matches /files/1 and /files/1/2. When tree_rm goes to delete
these two nodes, it first deletes (frees) /files/1 and all its
descendents. By the time we try to delete /files/1/2, the pointer we have
to that is no longer valid and we end up causing a double-free.

With this change, we make sure we only delete a node if none of its
ancestors is being deleted beforehand in the same operation - deleting a
node, and one of its ancestors afterwards is fine as the pointer to the
ancestor is still valid.

Fixes hercules-team#319

Special shoutout to Geoff Williams for finding, diagnosing and filing a
great bug report about this.

src/augeas.c

@@ -1142,6 +1142,7 @@ int tree_rm(struct pathx *p) {
     struct tree *tree, **del;
     int cnt = 0, ndel = 0, i;
 
+    /* set ndel to the number of trees we could possibly delete */
     for (tree = pathx_first(p); tree != NULL; tree = pathx_next(p)) {
         if (! TREE_HIDDEN(tree))
             ndel += 1;
@@ -1159,12 +1160,32 @@ int tree_rm(struct pathx *p) {
         if (TREE_HIDDEN(tree))
             continue;
         pathx_symtab_remove_descendants(pathx_get_symtab(p), tree);
-        del[i] = tree;
-        i += 1;
+        /* Collect the tree nodes that actually need to be deleted in
+           del. Mark the root of every subtree we are going to free by
+           setting tree->added. Only add a node to del if none of its
+           ancestors would have been freed by the time we get to freeing
+           that node; this avoids double frees for situations where the
+           path expression matches both /node and /node/child as unlinking
+           /node implicitly unlinks /node/child */
+        int live = 1;
+        for (struct tree *t = tree; live && ! ROOT_P(t); t = t->parent) {
+            if (t->added)
+                live = 0;
+        }
+        if (live) {
+            del[i] = tree;
+            i += 1;
+            tree->added = 1;
+        }
     }
+    /* ndel now means: the number of trees we are actually going to delete */
+    ndel = i;
 
-    for (i = 0; i < ndel; i++)
-        cnt += tree_unlink_raw(del[i]);
+    for (i = 0; i < ndel; i++) {
+        if (del[i] != NULL) {
+            cnt += tree_unlink_raw(del[i]);
+        }
+    }
     free(del);
 
     return cnt;

src/internal.h

@@ -378,7 +378,8 @@ struct tree {
     struct tree *children;   /* List of children through NEXT */
     char        *value;
     int          dirty;
-    uint8_t      added;      /* only used by ns_add to dedupe nodesets */
+    uint8_t      added;      /* only used by ns_add and tree_rm to dedupe
+                                nodesets */
     struct span *span;
 };
 

tests/test-api.c

@@ -642,6 +642,20 @@ static void testAugEscape(CuTest *tc) {
     free(out);
 }
 
+static void testRm(CuTest *tc) {
+    struct augeas *aug;
+    int r;
+
+    aug = aug_init(root, loadpath, AUG_NO_STDINC|AUG_NO_MODL_AUTOLOAD);
+    CuAssertPtrNotNull(tc, aug);
+
+    r = aug_set(aug, "/files/1/2/3/4/5", "1");
+    CuAssertRetSuccess(tc, r);
+
+    r = aug_rm(aug, "/files//*");
+    CuAssertIntEquals(tc, 5, r);
+}
+
 int main(void) {
     char *output = NULL;
     CuSuite* suite = CuSuiteNew();
@@ -661,6 +675,7 @@ int main(void) {
     SUITE_ADD_TEST(suite, testTextStore);
     SUITE_ADD_TEST(suite, testTextRetrieve);
     SUITE_ADD_TEST(suite, testAugEscape);
+    SUITE_ADD_TEST(suite, testRm);
 
     abs_top_srcdir = getenv("abs_top_srcdir");
     if (abs_top_srcdir == NULL)