From: Arran Cudbard-Bell <a.cudbardb@freeradius.org>
Date: Mon, 24 Apr 2017 18:54:58 +0000 (-0400)
Subject: Fixup how memory is freed in rbtrees
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6d2ab2d4514fb4f3135f6da9d422343e9505d34d;p=thirdparty%2Ffreeradius-server.git

Fixup how memory is freed in rbtrees
---

diff --git a/src/include/rbtree.h b/src/include/rbtree.h
index b711a309bec..eea96d6a2fe 100644
--- a/src/include/rbtree.h
+++ b/src/include/rbtree.h
@@ -51,7 +51,6 @@ typedef void (*rb_free_t)(void *data);
 
 rbtree_t	*rbtree_create(TALLOC_CTX *ctx, rb_comparator_t compare, rb_free_t node_free, int flags);
 void		rbtree_node_talloc_free(void *data);
-void		rbtree_free(rbtree_t *tree);
 bool		rbtree_insert(rbtree_t *tree, void *data);
 rbnode_t	*rbtree_insert_node(rbtree_t *tree, void *data);
 void		rbtree_delete(rbtree_t *tree, rbnode_t *z);
diff --git a/src/lib/ldap/libfreeradius-ldap.c b/src/lib/ldap/libfreeradius-ldap.c
index b4d93be534e..2533fd15dfd 100644
--- a/src/lib/ldap/libfreeradius-ldap.c
+++ b/src/lib/ldap/libfreeradius-ldap.c
@@ -965,6 +965,8 @@ static int _mod_conn_free(ldap_handle_t *conn)
 
 	rad_assert(conn->handle);
 
+	talloc_free_children(conn);	/* Force inverted free order */
+
 	fr_ldap_control_clear(conn);
 
 #ifdef HAVE_LDAP_UNBIND_EXT_S
diff --git a/src/lib/util/packet.c b/src/lib/util/packet.c
index 907865aeb74..269a36ec1ba 100644
--- a/src/lib/util/packet.c
+++ b/src/lib/util/packet.c
@@ -450,7 +450,7 @@ void fr_packet_list_free(fr_packet_list_t *pl)
 {
 	if (!pl) return;
 
-	rbtree_free(pl->tree);
+	talloc_free(pl->tree);
 	talloc_free(pl);
 }
 
diff --git a/src/lib/util/rbtree.c b/src/lib/util/rbtree.c
index e39e374246f..0a0664849b7 100644
--- a/src/lib/util/rbtree.c
+++ b/src/lib/util/rbtree.c
@@ -54,6 +54,9 @@ struct rbtree_t {
 	bool			replace;
 	bool			lock;
 	pthread_mutex_t		mutex;
+
+	TALLOC_CTX		*node_ctx;	//!< Freed last by the destructor, to ensure
+						//!< the tree is still functional.
 };
 
 #ifndef NDEBUG
@@ -83,19 +86,20 @@ void rbtree_node_talloc_free(void *data)
 	talloc_free(data);
 }
 
-/** Executes the free walker on a tree, and then frees the tree itself
+/** Free the rbtree cleaning up any nodes
+ *
+ * Walk the tree deleting nodes, then free any children of the tree.
  *
- * @note If you don't require the free walker to execute, you can just
- *	talloc_free the tree.  All mutexes will be cleaned up.
+ * @note If the destructor of a talloc descendent needs to lookup any
+ *	information in the tree, it will be unavailable at the point
+ *	of freeing.  We could fix this by introducing a pre-free callback
+ *	which gets called before any of the nodes are deleted.
  *
- * @param tree to free.
+ * @param[in] tree to tree.
+ * @return 0
  */
-void rbtree_free(rbtree_t *tree)
+static int _tree_free(rbtree_t *tree)
 {
-	if (!tree) return;
-
-	if (tree->lock) pthread_mutex_lock(&tree->mutex);
-
 	/*
 	 *	walk the tree, deleting the nodes...
 	 */
@@ -106,13 +110,16 @@ void rbtree_free(rbtree_t *tree)
 #endif
 	tree->root = NULL;
 
-	if (tree->lock) pthread_mutex_unlock(&tree->mutex);
-
-	talloc_free(tree);
-}
+	/*
+	 *	Ensure all dependents on the tree run their
+	 *	destructors.  The tree at this point should
+	 *	and any tree operations should be empty.
+	 */
+	talloc_free_children(tree);
 
-static int _rbtree_free(rbtree_t *tree)
-{
+	/*
+	 *	Clear up locks.
+	 */
 	if (tree->lock) pthread_mutex_destroy(&tree->mutex);
 
 	return 0;
@@ -120,6 +127,7 @@ static int _rbtree_free(rbtree_t *tree)
 
 /** Create a new RED-BLACK tree
  *
+ * @note Due to the node memory being allocated from a different pool to the main
  */
 rbtree_t *rbtree_create(TALLOC_CTX *ctx, rb_comparator_t compare, rb_free_t node_free, int flags)
 {
@@ -137,11 +145,10 @@ rbtree_t *rbtree_create(TALLOC_CTX *ctx, rb_comparator_t compare, rb_free_t node
 	tree->compare = compare;
 	tree->replace = (flags & RBTREE_FLAG_REPLACE) != 0 ? true : false;
 	tree->lock = (flags & RBTREE_FLAG_LOCK) != 0 ? true : false;
-	if (tree->lock) {
-		pthread_mutex_init(&tree->mutex, NULL);
-	}
+	tree->node_ctx = talloc_new(tree);
+	if (tree->lock) pthread_mutex_init(&tree->mutex, NULL);
 
-	talloc_set_destructor(tree, _rbtree_free);
+	talloc_set_destructor(tree, _tree_free);
 	tree->free = node_free;
 
 	return tree;
@@ -271,6 +278,8 @@ rbnode_t *rbtree_insert_node(rbtree_t *tree, void *data)
 {
 	rbnode_t *current, *parent, *x;
 
+	if (!tree->root) return NULL;
+
 	if (tree->lock) pthread_mutex_lock(&tree->mutex);
 
 	/* find where node belongs */
@@ -306,7 +315,7 @@ rbnode_t *rbtree_insert_node(rbtree_t *tree, void *data)
 	}
 
 	/* setup new node */
-	x = talloc_zero(tree, rbnode_t);
+	x = talloc_zero(tree->node_ctx, rbnode_t);
 	if (!x) {
 		fr_strerror_printf("No memory for new rbtree node");
 		if (tree->lock) pthread_mutex_unlock(&tree->mutex);
@@ -340,6 +349,8 @@ rbnode_t *rbtree_insert_node(rbtree_t *tree, void *data)
 
 bool rbtree_insert(rbtree_t *tree, void *data)
 {
+	if (!tree->root) return NULL;
+
 	if (rbtree_insert_node(tree, data)) return true;
 	return false;
 }
@@ -497,7 +508,11 @@ static void rbtree_delete_internal(rbtree_t *tree, rbnode_t *z, bool skiplock)
 		if (tree->lock) pthread_mutex_unlock(&tree->mutex);
 	}
 }
-void rbtree_delete(rbtree_t *tree, rbnode_t *z) {
+
+void rbtree_delete(rbtree_t *tree, rbnode_t *z)
+{
+	if (!tree->root) return;
+
 	rbtree_delete_internal(tree, z, false);
 }
 
@@ -507,8 +522,11 @@ void rbtree_delete(rbtree_t *tree, rbnode_t *z) {
  */
 bool rbtree_deletebydata(rbtree_t *tree, void const *data)
 {
-	rbnode_t *node = rbtree_find(tree, data);
+	rbnode_t *node;
+
+	if (!tree->root) return false;
 
+	node = rbtree_find(tree, data);
 	if (!node) return false;
 
 	rbtree_delete(tree, node);
@@ -524,6 +542,8 @@ rbnode_t *rbtree_find(rbtree_t *tree, void const *data)
 {
 	rbnode_t *current;
 
+	if (!tree->root) return NULL;
+
 	if (tree->lock) pthread_mutex_lock(&tree->mutex);
 	current = tree->root;
 
@@ -550,6 +570,8 @@ void *rbtree_finddata(rbtree_t *tree, void const *data)
 {
 	rbnode_t *x;
 
+	if (!tree->root) return NULL;
+
 	x = rbtree_find(tree, data);
 	if (!x) return NULL;
 
diff --git a/src/main/conf_file.c b/src/main/conf_file.c
index cb5b908d2e4..2f0310a92e5 100644
--- a/src/main/conf_file.c
+++ b/src/main/conf_file.c
@@ -586,19 +586,19 @@ static int _cf_section_free(CONF_SECTION *cs)
 	 *	cs.
 	 */
 	if (cs->pair_tree) {
-		rbtree_free(cs->pair_tree);
+		talloc_free(cs->pair_tree);
 		cs->pair_tree = NULL;
 	}
 	if (cs->section_tree) {
-		rbtree_free(cs->section_tree);
+		talloc_free(cs->section_tree);
 		cs->section_tree = NULL;
 	}
 	if (cs->name2_tree) {
-		rbtree_free(cs->name2_tree);
+		talloc_free(cs->name2_tree);
 		cs->name2_tree = NULL;
 	}
 	if (cs->data_tree) {
-		rbtree_free(cs->data_tree);
+		talloc_free(cs->data_tree);
 		cs->data_tree = NULL;
 	}
 
diff --git a/src/main/modules.c b/src/main/modules.c
index 4b082f11e7d..1796ce1098c 100644
--- a/src/main/modules.c
+++ b/src/main/modules.c
@@ -522,7 +522,7 @@ static void _module_thread_inst_tree_free(void *to_free)
 {
 	rbtree_t *thread_inst_tree = talloc_get_type_abort(to_free , rbtree_t);
 
-	rbtree_free(thread_inst_tree);
+	talloc_free(thread_inst_tree);
 }
 
 /** Compare two thread instances based on inst pointer
diff --git a/src/main/process.c b/src/main/process.c
index e08411fdb41..9092cb4f70c 100644
--- a/src/main/process.c
+++ b/src/main/process.c
@@ -5593,7 +5593,7 @@ void radius_event_free(void)
 		}
 	}
 
-	rbtree_free(pl);
+	talloc_free(pl);
 	pl = NULL;
 
 #ifdef WITH_PROXY
diff --git a/src/main/radclient.c b/src/main/radclient.c
index 129190ed9ad..2037621e5f7 100644
--- a/src/main/radclient.c
+++ b/src/main/radclient.c
@@ -1613,7 +1613,7 @@ int main(int argc, char **argv)
 		}
 	} while (!done);
 
-	rbtree_free(filename_tree);
+	talloc_free(filename_tree);
 	fr_packet_list_free(pl);
 	while (request_head) TALLOC_FREE(request_head);
 	talloc_free(dict);
diff --git a/src/main/realms.c b/src/main/realms.c
index 88f5004c66a..ae6ed49dc0b 100644
--- a/src/main/realms.c
+++ b/src/main/realms.c
@@ -296,21 +296,21 @@ void realms_free(void)
 {
 #ifdef WITH_PROXY
 #  ifdef WITH_STATS
-	rbtree_free(home_servers_bynumber);
+	talloc_free(home_servers_bynumber);
 	home_servers_bynumber = NULL;
 #  endif
 
-	rbtree_free(home_servers_byname);
+	talloc_free(home_servers_byname);
 	home_servers_byname = NULL;
 
-	rbtree_free(home_servers_byaddr);
+	talloc_free(home_servers_byaddr);
 	home_servers_byaddr = NULL;
 
-	rbtree_free(home_pools_byname);
+	talloc_free(home_pools_byname);
 	home_pools_byname = NULL;
 #endif
 
-	rbtree_free(realms_byname);
+	talloc_free(realms_byname);
 	realms_byname = NULL;
 
 	realm_pool_free(NULL);
diff --git a/src/main/state.c b/src/main/state.c
index 0c919a8c97b..8e3c81dcf86 100644
--- a/src/main/state.c
+++ b/src/main/state.c
@@ -163,7 +163,7 @@ static int _state_tree_free(fr_state_tree_t *state)
 	/*
 	 *	Free the rbtree
 	 */
-	rbtree_free(state->tree);
+	talloc_free(state->tree);
 
 	if (state == global_state) global_state = NULL;
 
diff --git a/src/modules/rlm_cache/drivers/rlm_cache_rbtree/rlm_cache_rbtree.c b/src/modules/rlm_cache/drivers/rlm_cache_rbtree/rlm_cache_rbtree.c
index c22d9fcc862..2de9e0c3b3c 100644
--- a/src/modules/rlm_cache/drivers/rlm_cache_rbtree/rlm_cache_rbtree.c
+++ b/src/modules/rlm_cache/drivers/rlm_cache_rbtree/rlm_cache_rbtree.c
@@ -93,7 +93,7 @@ static int mod_detach(void *instance)
 	if (driver->heap) fr_heap_delete(driver->heap);
 	if (driver->cache) {
 		rbtree_walk(driver->cache, RBTREE_DELETE_ORDER, _cache_entry_free, NULL);
-		rbtree_free(driver->cache);
+		talloc_free(driver->cache);
 	}
 
 	pthread_mutex_destroy(&driver->mutex);
diff --git a/src/modules/rlm_files/rlm_files.c b/src/modules/rlm_files/rlm_files.c
index 7d7f52dff8b..aac24dbd165 100644
--- a/src/modules/rlm_files/rlm_files.c
+++ b/src/modules/rlm_files/rlm_files.c
@@ -222,7 +222,7 @@ static int getusersfile(TALLOC_CTX *ctx, char const *filename, rbtree_t **ptree)
 				error:
 					pairlist_free(&entry);
 					pairlist_free(&next);
-					rbtree_free(tree);
+					talloc_free(tree);
 					return -1;
 				}
 
diff --git a/src/modules/rlm_python/rlm_python.c b/src/modules/rlm_python/rlm_python.c
index e22e2c4bd89..0638c399f77 100644
--- a/src/modules/rlm_python/rlm_python.c
+++ b/src/modules/rlm_python/rlm_python.c
@@ -602,7 +602,7 @@ static void _python_thread_tree_free(void *arg)
 	rad_assert(arg == local_thread_state);
 
 	rbtree_t *tree = talloc_get_type_abort(arg, rbtree_t);
-	rbtree_free(tree);	/* Needs to be this not talloc_free to execute delete walker */
+	talloc_free(tree);	/* Needs to be this not talloc_free to execute delete walker */
 
 	local_thread_state = NULL;	/* Prevent double free in unit_test_module env */
 }
@@ -1068,7 +1068,7 @@ static int mod_detach(void *instance)
 	 *	unit_test_module framework, and probably with the server running
 	 *	in debug mode.
 	 */
-	rbtree_free(local_thread_state);
+	talloc_free(local_thread_state);
 	local_thread_state = NULL;
 
 	/*
diff --git a/src/modules/rlm_securid/rlm_securid.c b/src/modules/rlm_securid/rlm_securid.c
index 2479df3560a..f5e687faf38 100644
--- a/src/modules/rlm_securid/rlm_securid.c
+++ b/src/modules/rlm_securid/rlm_securid.c
@@ -410,7 +410,7 @@ static int mod_detach(void *instance)
 
 	/* delete session tree */
 	if (inst->session_tree) {
-		rbtree_free(inst->session_tree);
+		talloc_free(inst->session_tree);
 		inst->session_tree = NULL;
 	}
 
diff --git a/src/modules/rlm_sigtran/sccp.c b/src/modules/rlm_sigtran/sccp.c
index 349ca8981d0..512ef03f660 100644
--- a/src/modules/rlm_sigtran/sccp.c
+++ b/src/modules/rlm_sigtran/sccp.c
@@ -396,6 +396,6 @@ void sigtran_sccp_global_free(void)
 {
 	if (--txn_tree_inst > 0) return;
 
-	rbtree_free(txn_tree);
+	talloc_free(txn_tree);
 	txn_tree = NULL;
 }
diff --git a/src/tests/rbmonkey.c b/src/tests/rbmonkey.c
index f2ac1d39c28..38cae031d47 100644
--- a/src/tests/rbmonkey.c
+++ b/src/tests/rbmonkey.c
@@ -227,7 +227,7 @@ again:
 		}
 	}
 	fprintf(stderr,"matched OK\n");
-	rbtree_free(t);
+	talloc_free(t);
 	goto again;
 
 bad: