Temporary fix for access after free in dl loader

diff --git a/src/include/dl.h b/src/include/dl.h
index f11ff3f2cc793b1237066b0acaf2f142c8f5ed79..cfeca192d02869cd5d44f7997c396c47c1b96079 100644 (file)
--- a/src/include/dl.h
+++ b/src/include/dl.h
@@ -94,19 +94,19 @@ typedef int (*module_detach_t)(void *instance);
  *
  * @param[in] module   being loaded.
  * @param[in] symbol   which, if present, will trigger this callback.
- * @param[in] user_ctx passed to dl_init_register.
+ * @param[in] user_ctx passed to dl_loader_init_register.
  * @return
  *     - 0 on success.
  *     - -1 on failure
  */
-typedef int (*dl_init_t)(dl_t const *module, void *symbol, void *user_ctx);
+typedef int (*dl_loader_init_t)(dl_t const *module, void *symbol, void *user_ctx);
 
 
 /** Callback when a module is destroyed
  *
  * @param[in] module   being loaded.
  * @param[in] symbol   which, if present, will trigger this callback.
- * @param[in] user_ctx passed to dl_init_register
+ * @param[in] user_ctx passed to dl_loader_init_register
  */
 typedef void (*dl_free_t)(dl_t const *module, void *symbol, void *user_ctx);
 
@@ -166,12 +166,12 @@ struct dl_instance {
 #define DL_INSTANTIATE_PRIORITY        5               //!< Callback priority for bootstrap callback
 
 
-int                    dl_init(TALLOC_CTX *ctx, char const *lib_dir);
+int                    dl_loader_init(TALLOC_CTX *ctx, char const *lib_dir);
 
 int                    dl_symbol_init_cb_register(unsigned int priority, char const *symbol,
-                                                  dl_init_t func, void *ctx);
+                                                  dl_loader_init_t func, void *ctx);
 
-void                   dl_symbol_init_cb_unregister(char const *symbol, dl_init_t func);
+void                   dl_symbol_init_cb_unregister(char const *symbol, dl_loader_init_t func);
 
 int                    dl_symbol_free_cb_register(unsigned int priority, char const *symbol,
                                                   dl_free_t func, void *ctx);

diff --git a/src/main/dl.c b/src/main/dl.c
index 6a19e8f0785dcc1c16a31071b3b96700d82bfc79..8174091e555f4aae5e8c99aecafa029634559cb0 100644 (file)
--- a/src/main/dl.c
+++ b/src/main/dl.c
@@ -61,7 +61,7 @@ typedef struct dl_symbol_init dl_symbol_init_t;
 struct dl_symbol_init {
        unsigned int            priority;       //!< Call priority
        char const              *symbol;        //!< to search for.  May be NULL in which case func is always called.
-       dl_init_t               func;           //!< to call when symbol is found in a module's symbol table.
+       dl_loader_init_t        func;           //!< to call when symbol is found in a module's symbol table.
        void                    *ctx;           //!< User data to pass to func.
        dl_symbol_init_t        *next;
 };
@@ -109,7 +109,8 @@ typedef struct dl_loader {
         */
        rbtree_t                *tree;
 } dl_loader_t;
-static dl_loader_t *dl;
+
+static dl_loader_t *dl_loader;
 
 /** Name prefixes matching the types of loadable module
  */
@@ -285,7 +286,7 @@ static int dl_symbol_init_walk(dl_t const *dl_module)
        void                    *sym = NULL;
        char                    buffer[256];
 
-       for (init = fr_cursor_init(&cursor, &dl->sym_init);
+       for (init = fr_cursor_init(&cursor, &dl_loader->sym_init);
             init;
             init = fr_cursor_next(&cursor)) {
                if (init->symbol) {
@@ -320,7 +321,7 @@ static void dl_symbol_free_walk(dl_t const *dl_module)
        fr_cursor_t             cursor;
        void                    *sym = NULL;
 
-       for (free = fr_cursor_init(&cursor, &dl->sym_free);
+       for (free = fr_cursor_init(&cursor, &dl_loader->sym_free);
             free;
             free = fr_cursor_next(&cursor)) {
                if (free->symbol) {
@@ -364,14 +365,14 @@ static int _dl_free(dl_t *module)
 
        module->handle = NULL;
 
-       rbtree_deletebydata(dl->tree, module);
+       rbtree_deletebydata(dl_loader->tree, module);
 
        /*
         *      If everything has been freed, autofree the tree.
         *      dl *MUST* be set to NULL, so that if the server decides to
         *      load more modules, the tree is recreated.
         */
-       if (rbtree_num_elements(dl->tree) == 0) TALLOC_FREE(dl);
+       if (rbtree_num_elements(dl_loader->tree) == 0) TALLOC_FREE(dl_loader);
 
        return 0;
 }
@@ -393,20 +394,20 @@ static int _dl_free(dl_t *module)
  *     - 0 on success (or already registered).
  *     - -1 on failure.
  */
-int dl_symbol_init_cb_register(unsigned int priority, char const *symbol, dl_init_t func, void *ctx)
+int dl_symbol_init_cb_register(unsigned int priority, char const *symbol, dl_loader_init_t func, void *ctx)
 {
        dl_symbol_init_t        *n, *p;
        fr_cursor_t             cursor;
 
        dl_symbol_init_cb_unregister(symbol, func);
 
-       MEM(n = talloc(dl, dl_symbol_init_t));
+       MEM(n = talloc(dl_loader, dl_symbol_init_t));
        n->priority = priority;
        n->symbol = symbol;
        n->func = func;
        n->ctx = ctx;
 
-       for (p = fr_cursor_init(&cursor, &dl->sym_init); p && (p->priority >= priority); fr_cursor_next(&cursor));
+       for (p = fr_cursor_init(&cursor, &dl_loader->sym_init); p && (p->priority >= priority); fr_cursor_next(&cursor));
        fr_cursor_insert(&cursor, n);
 
        return 0;
@@ -417,7 +418,7 @@ int dl_symbol_init_cb_register(unsigned int priority, char const *symbol, dl_ini
  * @param[in] symbol   the callback is attached to.
  * @param[in] func     the callback.
  */
-void dl_symbol_init_cb_unregister(char const *symbol, dl_init_t func)
+void dl_symbol_init_cb_unregister(char const *symbol, dl_loader_init_t func)
 {
        dl_symbol_init_t        *found, find;
        fr_cursor_t     cursor;
@@ -425,7 +426,7 @@ void dl_symbol_init_cb_unregister(char const *symbol, dl_init_t func)
        find.symbol = symbol;
        find.func = func;
 
-       for (found = fr_cursor_init(&cursor, &dl->sym_init);
+       for (found = fr_cursor_init(&cursor, &dl_loader->sym_init);
             found && (dl_symbol_init_cmp(&find, found) != 0);
             found = fr_cursor_next(&cursor));
 
@@ -456,13 +457,13 @@ int dl_symbol_free_cb_register(unsigned int priority, char const *symbol, dl_fre
 
        dl_symbol_free_cb_unregister(symbol, func);
 
-       MEM(n = talloc(dl, dl_symbol_free_t));
+       MEM(n = talloc(dl_loader, dl_symbol_free_t));
        n->priority = priority;
        n->symbol = symbol;
        n->func = func;
        n->ctx = ctx;
 
-       for (p = fr_cursor_init(&cursor, &dl->sym_free); p && (p->priority >= priority); fr_cursor_next(&cursor));
+       for (p = fr_cursor_init(&cursor, &dl_loader->sym_free); p && (p->priority >= priority); fr_cursor_next(&cursor));
        fr_cursor_insert(&cursor, n);
 
        return 0;
@@ -481,7 +482,7 @@ void dl_symbol_free_cb_unregister(char const *symbol, dl_free_t func)
        find.symbol = symbol;
        find.func = func;
 
-       for (found = fr_cursor_init(&cursor, &dl->sym_free);
+       for (found = fr_cursor_init(&cursor, &dl_loader->sym_free);
             found && (dl_symbol_free_cmp(&find, found) != 0);
             found = fr_cursor_next(&cursor));
 
@@ -495,7 +496,7 @@ dl_instance_t const *dl_instance_find(void *data)
 {
        dl_instance_t find = { .data = data };
 
-       return rbtree_finddata(dl->inst_tree, &find);
+       return rbtree_finddata(dl_loader->inst_tree, &find);
 }
 
 /** Allocate module instance data, and parse the module's configuration
@@ -557,7 +558,7 @@ void *dl_by_name(char const *name)
                DEBUG3("Ignoring libdir as FR_LIBRARY_PATH set.  Module search path will be: %s", env);
                search_path = env;
        } else {
-               search_path = dl->lib_dir;
+               search_path = dl_loader->lib_dir;
        }
 
        /*
@@ -705,7 +706,7 @@ dl_t const *dl_module(CONF_SECTION *conf, dl_t const *parent, char const *name,
        /*
         *      If the module's already been loaded, increment the reference count.
         */
-       dl_module = rbtree_finddata(dl->tree, &to_find);
+       dl_module = rbtree_finddata(dl_loader->tree, &to_find);
        if (dl_module) {
                talloc_free(module_name);
                talloc_increase_ref_count(dl_module);
@@ -743,7 +744,7 @@ dl_t const *dl_module(CONF_SECTION *conf, dl_t const *parent, char const *name,
        DEBUG3("%s validated.  Handle address %p, symbol address %p", module_name, handle, module);
 
        /* make room for the module type */
-       dl_module = talloc_zero(dl->tree, dl_t);
+       dl_module = talloc_zero(dl_loader->tree, dl_t);
        dl_module->parent = parent;
        dl_module->common = module;
        dl_module->handle = handle;
@@ -763,7 +764,7 @@ dl_t const *dl_module(CONF_SECTION *conf, dl_t const *parent, char const *name,
        /*
         *      Add the module to the dlhandle cache
         */
-       if (!rbtree_insert(dl->tree, dl_module)) {
+       if (!rbtree_insert(dl_loader->tree, dl_module)) {
                cf_log_err(conf, "Failed to cache module \"%s\"", module_name);
                goto error;
        }
@@ -790,7 +791,7 @@ static int _dl_instance_free(dl_instance_t *dl_inst)
        /*
         *      Remove this instance from the tracking tree.
         */
-       rbtree_deletebydata(dl->inst_tree, dl_inst);
+       rbtree_deletebydata(dl_loader->inst_tree, dl_inst);
 
        /*
         *      Ensure sane free order, and that all destructors
@@ -851,6 +852,7 @@ int dl_instance(TALLOC_CTX *ctx, dl_instance_t **out,
                char const *name, dl_type_t type)
 {
        dl_instance_t   *dl_inst;
+       char const      *name2;
 
        MEM(dl_inst = talloc_zero(ctx, dl_instance_t));
        talloc_set_destructor(dl_inst, _dl_instance_free);
@@ -884,53 +886,113 @@ int dl_instance(TALLOC_CTX *ctx, dl_instance_t **out,
                }
        }
 
+       name2 = cf_section_name2(conf);
+       if (name2) {
+               dl_inst->name = talloc_typed_strdup(dl_inst, name2);
+       } else {
+               dl_inst->name = talloc_typed_strdup(dl_inst, cf_section_name1(conf));
+       }
+
        dl_inst->conf = conf;
        dl_inst->parent = parent;
 
-       rbtree_insert(dl->inst_tree, dl_inst);  /* Duplicates not possible */
+       rbtree_insert(dl_loader->inst_tree, dl_inst);   /* Duplicates not possible */
 
        *out = dl_inst;
 
        return 0;
 }
 
+static int _dl_walk_print(UNUSED void *context, void *data)
+{
+       dl_t *dl = talloc_get_type_abort(data, dl_t);
+
+       WARN("  %s", dl->name);
+
+       return 0;
+}
+
+static int _dl_inst_walk_print(UNUSED void *context, void *data)
+{
+       dl_instance_t *dl_instance = talloc_get_type_abort(data, dl_instance_t);
+
+       WARN("  %s", dl_instance->name);
+
+       return 0;
+}
+
+static int _dl_loader_free(dl_loader_t *dl_l)
+{
+       int ret = 0;
+
+       /*
+        *      Prevent freeing if we still have modules loaded
+        *      We do reference counting, we know exactly what
+        *      should still be active.
+        */
+       if (rbtree_num_elements(dl_l->tree) > 0) {
+               ret = -1;
+#ifndef NDEBUG
+               WARN("Refusing to cleanup dl loader, the following modules are still in use:");
+               rbtree_walk(dl_l->tree, RBTREE_IN_ORDER, _dl_walk_print, NULL);
+#endif
+
+       }
+
+       if (rbtree_num_elements(dl_l->inst_tree) > 0) {
+               ret = -1;
+#ifndef NDEBUG
+               WARN("Refusing to cleanup dl loader, the following module instances are still in use:");
+               rbtree_walk(dl_l->inst_tree, RBTREE_IN_ORDER, _dl_inst_walk_print, NULL);
+#endif
+       }
+
+       if (ret != 0) WARN("This may appear as a leak in talloc memory reports");
+
+       return ret;
+}
+
 /** Initialise structures needed by the dynamic linker
  *
  */
-int dl_init(TALLOC_CTX *ctx, char const *lib_dir)
+int dl_loader_init(TALLOC_CTX *ctx, char const *lib_dir)
 {
-       if (dl) return 0;
+       if (dl_loader) return 0;
 
-       dl = talloc_zero(ctx, dl_loader_t);
-       dl->tree = rbtree_talloc_create(dl, dl_handle_cmp, dl_t, NULL, 0);
-       if (!dl->tree) {
+       dl_loader = talloc_zero(ctx, dl_loader_t);
+       dl_loader->tree = rbtree_talloc_create(dl_loader, dl_handle_cmp, dl_t, NULL, 0);
+       if (!dl_loader->tree) {
                ERROR("Failed initialising dl->tree");
+       error:
+               talloc_free(dl_loader);
                return -1;
        }
 
-       dl->inst_tree = rbtree_talloc_create(dl, dl_inst_cmp, dl_instance_t, NULL, 0);
-       if (!dl->inst_tree) {
+       dl_loader->inst_tree = rbtree_talloc_create(dl_loader, dl_inst_cmp, dl_instance_t, NULL, 0);
+       if (!dl_loader->inst_tree) {
                ERROR("Failed initialising dl->inst_tree");
-               return -1;
+               goto error;
        }
 
        if (dl_symbol_init_cb_register(DL_INSTANTIATE_PRIORITY, NULL, dl_load_func, NULL) < 0) {
                ERROR("Failed registering load() callback");
-               return -1;
+               goto error;
        }
 
        if (dl_symbol_free_cb_register(DL_INSTANTIATE_PRIORITY, NULL, dl_unload_func, NULL) < 0) {
                ERROR("Failed registering unload() callback");
-               return -1;
+               goto error;
        }
 
        if (lib_dir) {
-               dl->lib_dir = talloc_strdup(dl, lib_dir);
-               if (!dl->lib_dir) {
+               dl_loader->lib_dir = talloc_strdup(dl_loader, lib_dir);
+               if (!dl_loader->lib_dir) {
                        ERROR("Failed recording log dir");
-                       return -1;
+                       goto error;
                }
        }
 
+       talloc_set_destructor(dl_loader, _dl_loader_free);
+
        return 0;
 }

diff --git a/src/main/radiusd.c b/src/main/radiusd.c
index 5bd4e5e4adaabd9e0060b0ade3dbe0600f56424c..b1c730c63ba349781bd3ad2a272b6b81155ec7a4 100644 (file)
--- a/src/main/radiusd.c
+++ b/src/main/radiusd.c
@@ -409,7 +409,7 @@ int main(int argc, char *argv[])
         *      Initialize the DL infrastructure, which is used by the
         *      config file parser.
         */
-       dl_init(autofree, config->lib_dir);
+       dl_loader_init(autofree, config->lib_dir);
 
        /*
         *      Initialise the top level dictionary hashes which hold

diff --git a/src/main/unit_test_module.c b/src/main/unit_test_module.c
index be5428ab0b7f209c781be8fa1ff69924f27b3f37..08aa53bfb8e45cd6287e51dec7523417db2da7ba 100644 (file)
--- a/src/main/unit_test_module.c
+++ b/src/main/unit_test_module.c
@@ -812,7 +812,7 @@ int main(int argc, char *argv[])
                exit(EXIT_FAILURE);
        }
 
-       dl_init(autofree, config->lib_dir);
+       dl_loader_init(autofree, config->lib_dir);
 
        if (fr_dict_global_init(autofree, config->dict_dir) < 0) {
                fr_perror("%s", config->name);
@@ -1179,11 +1179,12 @@ finish:
         */
        xlat_unregister("poke");
 
+       ERROR("MODULES FREE");
        /*
         *      Detach modules, connection pools, registered xlats / paircmps / maps.
         */
        modules_free();
-
+       ERROR("MODULES DONE");
        /*
         *      The only paircmps remaining are the ones registered by the server core.
         */