Fixup how memory is freed in rbtrees

diff --git a/src/include/rbtree.h b/src/include/rbtree.h
index b711a309bec59de70f0c8336a713ace2e56b3c62..eea96d6a2feb400c4917a6bf98a14e18fb58e0f7 100644 (file)
--- 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 b4d93be534e432091609d0176779021f3f75bc75..2533fd15dfd44a687178bebef59fa20347d7dbb1 100644 (file)
--- 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 907865aeb74b8c2dced0d6196e5b2863eaec9d14..269a36ec1bac9881c63e6780eff47e0a2ecdd0a6 100644 (file)
--- 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 e39e374246fa96c01cd827541ab465a8c5438aeb..0a0664849b773d922598e56a136909dbc4629cc3 100644 (file)
--- 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 cb5b908d2e4e814c43192ab0927c516cdd3e7113..2f0310a92e5ff1b4c04ede7388af3249f4521e69 100644 (file)
--- 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 4b082f11e7dc4130c90fe1c9e6652e075bab5e57..1796ce1098cd1c89e07a45aaec02d79b23064426 100644 (file)
--- 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 e08411fdb41ed57170c7e4bbb71dd50ac89f3ff8..9092cb4f70c005a40dfaa0b72b10c42bd8b44ce3 100644 (file)
--- 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 129190ed9add99bf2037b31cd04464d39c9ed078..2037621e5f71af15d0cd2628159a648ca1158ff3 100644 (file)
--- 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 88f5004c66a1602fa3f79ea15d615c552a49d2af..ae6ed49dc0b4dc8791f9d8b91593619c1856e307 100644 (file)
--- 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 0c919a8c97b7643c02a8de6dff8b86926d116e39..8e3c81dcf868861dc8d4846aec4cb58c8a13957e 100644 (file)
--- 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 c22d9fcc862f9f853a9d6f2a5be83ee108216800..2de9e0c3b3ce82e8cee8c4e05ffaafbb330c87d1 100644 (file)
--- 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 7d7f52dff8b810b7642ba87a6f1ccaad0658ce77..aac24dbd165d72f8f9e9b72c596f835d7cadbff7 100644 (file)
--- 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 e22e2c4bd898077e081b307f7f7a4f20f03f74ac..0638c399f775afc90d6efa23c19e03921bf9db1f 100644 (file)
--- 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 2479df3560a4f163df0ccecefe66cc341c8b46be..f5e687faf38d922f0bca75bc60fdafaf0981d03d 100644 (file)
--- 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 349ca8981d003ade7eacb0f4deb86416d22b4688..512ef03f6608ad887382c7847624be118d26f4a8 100644 (file)
--- 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 f2ac1d39c28868162f0e3c544b363baed38b2245..38cae031d47e9baa31023491761d82bad0b6010c 100644 (file)
--- 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: