update RB tree freeing process

There are conflicts between the behavior of "free the tree", and
the talloc destructors for a node.  For simplicity / laziness,
the "free tree walker" just walks over the tree, freeing the
node data.  It expects that the tree nodes themselves remain
active during this walk, as the tree is not rebalanced.

The free walker will free the node data, which should NOT free
the individual node.  The node may, in fact, be inside of the
block which is being freed!

We therefore free the talloc children before mangling the tree
structure, so that any talloc destructors can look at the "clean"
tree structure.

We update the various destructors for tree data to check if the
tree is being freed, and then don't try to find / remove the entry
in the tree.

We also update the allocations so that the nodes in the tree are
always parented from the tree.  That way they are cleaned up before
the tree is cleaned up.

If (as before) the tree and nodes are both parented from the same
parent, then the nodes / tree are freed in essentially random order,
and the nodes might stick around after the tree is freed

diff --git a/src/bin/radclient.c b/src/bin/radclient.c
index 43225be2aa93b719f3c1f8e9dc0499afdd8cc7d4..05282cc0e0a9840b81fc4bdb435f6b44f63c76aa 100644 (file)
--- a/src/bin/radclient.c
+++ b/src/bin/radclient.c
@@ -194,7 +194,9 @@ static int _rc_request_free(rc_request_t *request)
 {
        rc_request_list_remove(&rc_request_list, request);
 
-       if (do_coa) (void) fr_rb_delete_by_inline_node(coa_tree, &request->node);
+       if (do_coa && fr_rb_node_inline_in_tree(&request->node) && !coa_tree->being_freed) {
+               (void) fr_rb_delete_by_inline_node(coa_tree, &request->node);
+       }
 
        return 0;
 }

diff --git a/src/lib/io/master.c b/src/lib/io/master.c
index 44df7a0d3d754b75e140794c84027ab89238866a..f511d61cd2b305423bfd99a06c10f50075ea96b7 100644 (file)
--- a/src/lib/io/master.c
+++ b/src/lib/io/master.c
@@ -187,6 +187,16 @@ static int track_free(fr_io_track_t *track)
 static int track_dedup_free(fr_io_track_t *track)
 {
        fr_assert(track->client->table != NULL);
+
+       /*
+        *      If the tree is being freed, then we don't try to remove ourselves from it.  Doing so would
+        *      free this node, and therefore corrupt the tree.
+        *
+        *      The talloc code will take care of cleaning up the children and events when this chunk is
+        *      freed.
+        */
+       if (track->client->table->being_freed) return 0;
+
        fr_assert(fr_rb_find(track->client->table, track) != NULL);
 
        if (!fr_rb_delete(track->client->table, track)) {
@@ -1103,12 +1113,12 @@ static fr_io_track_t *fr_io_track_add(fr_listen_t const *li, fr_io_client_t *cli
         *      there are no duplicates, so this is fine.
         */
        if (client->connection) {
-               MEM(track = talloc_zero_pooled_object(client, fr_io_track_t, 1, sizeof(*track) + 64));
+               MEM(track = talloc_zero_pooled_object(client->table, fr_io_track_t, 1, sizeof(*track) + 64));
                track->address = client->connection->address;
        } else {
                fr_io_address_t *my_address;
 
-               MEM(track = talloc_zero_pooled_object(client, fr_io_track_t, 1, sizeof(*track) + sizeof(*track->address) + 64));
+               MEM(track = talloc_zero_pooled_object(client->table, fr_io_track_t, 1, sizeof(*track) + sizeof(*track->address) + 64));
                MEM(track->address = my_address = talloc(track, fr_io_address_t));
 
                *my_address = *address;
@@ -3344,7 +3354,7 @@ fr_io_track_t *fr_master_io_track_alloc(fr_listen_t *li, fr_client_t *radclient,
                MEM(client = client_alloc(thread, PR_CLIENT_STATIC, inst, thread, radclient, NULL));
        }
 
-       MEM(track = talloc_zero_pooled_object(client, fr_io_track_t, 1, sizeof(*track) + sizeof(*track->address) + 64));
+       MEM(track = talloc_zero_pooled_object(client->table, fr_io_track_t, 1, sizeof(*track) + sizeof(*track->address) + 64));
        MEM(track->address = address = talloc_zero(track, fr_io_address_t));
 
        track->li = li;

diff --git a/src/lib/server/module.c b/src/lib/server/module.c
index edaa452e558b9cf6fd0f8ba8005546d1eb09f12d..c26ba354ddff80d758663aa0c7747ab9b0253e91 100644 (file)
--- a/src/lib/server/module.c
+++ b/src/lib/server/module.c
@@ -1504,8 +1504,20 @@ static int _module_instance_free(module_instance_t *mi)
                return -1;
        }
 
-       if (fr_rb_node_inline_in_tree(&mi->name_node) && !fr_cond_assert(fr_rb_delete(ml->name_tree, mi))) return 1;
-       if (fr_rb_node_inline_in_tree(&mi->data_node) && !fr_cond_assert(fr_rb_delete(ml->data_tree, mi))) return 1;
+       /*
+        *      If the tree is being freed, then we don't try to remove ourselves from it.  Doing so would
+        *      free this node, and therefore corrupt the tree.
+        *
+        *      The talloc code will take care of cleaning up the children and events when this chunk is
+        *      freed.
+        */
+       if (!ml->name_tree->being_freed) {
+               if (fr_rb_node_inline_in_tree(&mi->name_node) && !fr_cond_assert(fr_rb_delete(ml->name_tree, mi))) return 1;
+       }
+
+       if (!ml->data_tree->being_freed) {
+               if (fr_rb_node_inline_in_tree(&mi->data_node) && !fr_cond_assert(fr_rb_delete(ml->data_tree, mi))) return 1;
+       }
        if (ml->type->data_del) ml->type->data_del(mi);
 
        /*

diff --git a/src/lib/util/rb.c b/src/lib/util/rb.c
index 1371e8abac99659ec2183ca6f314b9b9249d1bfd..291ee9f5238ab94e531edecbcb04c914c327636a 100644 (file)
--- a/src/lib/util/rb.c
+++ b/src/lib/util/rb.c
@@ -114,12 +114,6 @@ static int _tree_free(fr_rb_tree_t *tree)
         */
        if ((tree->root != NIL) && tree->data_free) free_walker(tree, tree->root);
 
-#ifndef NDEBUG
-       tree->magic = 0;
-#endif
-       tree->root = NIL;
-       tree->num_elements = 0;
-
        /*
         *      Ensure all dependents on the tree run their
         *      destructors.  The tree at this point should
@@ -127,6 +121,12 @@ static int _tree_free(fr_rb_tree_t *tree)
         */
        talloc_free_children(tree);
 
+#ifndef NDEBUG
+       tree->magic = 0;
+#endif
+       tree->root = NIL;
+       tree->num_elements = 0;
+
        return 0;
 }