From: Alan T. DeKok <aland@freeradius.org>
Date: Tue, 5 May 2026 16:29:36 +0000 (-0400)
Subject: update RB tree freeing process
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=bfa685675c689f20fe10600d06e95c0829d0bf84;p=thirdparty%2Ffreeradius-server.git

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 43225be2aa9..05282cc0e0a 100644
--- 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 44df7a0d3d7..f511d61cd2b 100644
--- 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 edaa452e558..c26ba354ddf 100644
--- 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 1371e8abac9..291ee9f5238 100644
--- 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;
 }