From 69b82113fa4ab0236b3c0eaf84710cc0e74eb7f6 Mon Sep 17 00:00:00 2001 From: Corey Farrell Date: Tue, 21 Nov 2017 21:34:56 -0500 Subject: loader: Minor fix to module registration. This protects the module loader itself against crashing if dlopen is called on a module from outside loader.c. * Expand scope of lock inside ast_module_register to include reading of resource_being_loaded. * NULL check resource_being_loaded. * Set resource_being_loaded NULL as soon as dlopen returns. This fixes some error paths where it was not NULL'ed. * Create module_destroy function to deduplicate code from ast_module_unregister and modules_shutdown. * Resolve leak that occured if a module did not successfully register. * Simplify checking for successful registration. Change-Id: I40f07a315e55b92df4fc7faf525ed6d4f396e7d2 --- main/loader.c | 107 +++++++++++++++++++++++++++++++--------------------------- 1 file changed, 57 insertions(+), 50 deletions(-) diff --git a/main/loader.c b/main/loader.c index 23a29d994..f06d637a8 100644 --- a/main/loader.c +++ b/main/loader.c @@ -167,38 +167,51 @@ static int do_full_reload = 0; static AST_DLLIST_HEAD_STATIC(reload_queue, reload_queue_item); -/* when dynamic modules are being loaded, ast_module_register() will - need to know what filename the module was loaded from while it - is being registered -*/ +/*! + * \internal + * + * This variable is set by load_dynamic_module so ast_module_register + * can know what pointer is being registered. + * + * This is protected by the module_list lock. + */ static struct ast_module *resource_being_loaded; -/* XXX: should we check for duplicate resource names here? */ - +/*! + * \internal + * \brief Used by AST_MODULE_INFO to register with the module loader. + * + * This function is automatically called when each module is opened. + * It must never be used from outside AST_MODULE_INFO. + */ void ast_module_register(const struct ast_module_info *info) { - struct ast_module *mod = resource_being_loaded; + struct ast_module *mod; + + /* + * This lock protects resource_being_loaded as well as the module + * list. Normally we already have a lock on module_list when we + * begin the load but locking again from here prevents corruption + * if an asterisk module is dlopen'ed from outside the module loader. + */ + AST_DLLIST_LOCK(&module_list); + mod = resource_being_loaded; + if (!mod) { + AST_DLLIST_UNLOCK(&module_list); + return; + } ast_debug(5, "Registering module %s\n", info->name); + /* This tells load_dynamic_module that we're registered. */ + resource_being_loaded = NULL; + mod->info = info; if (ast_opt_ref_debug) { mod->ref_debug = ao2_t_alloc(0, NULL, info->name); } AST_LIST_HEAD_INIT(&mod->users); - /* during startup, before the loader has been initialized, - there are no threads, so there is no need to take the lock - on this list to manipulate it. it is also possible that it - might be unsafe to use the list lock at that point... so - let's avoid it altogether - */ - AST_DLLIST_LOCK(&module_list); - /* it is paramount that the new entry be placed at the tail of - the list, otherwise the code that uses dlopen() to load - dynamic modules won't be able to find out if the module it - just opened was registered or failed to load - */ AST_DLLIST_INSERT_TAIL(&module_list, mod, entry); AST_DLLIST_UNLOCK(&module_list); @@ -206,6 +219,13 @@ void ast_module_register(const struct ast_module_info *info) *((struct ast_module **) &(info->self)) = mod; } +static void module_destroy(struct ast_module *mod) +{ + AST_LIST_HEAD_DESTROY(&mod->users); + ao2_cleanup(mod->ref_debug); + ast_free(mod); +} + void ast_module_unregister(const struct ast_module_info *info) { struct ast_module *mod = NULL; @@ -226,9 +246,7 @@ void ast_module_unregister(const struct ast_module_info *info) if (mod) { ast_debug(5, "Unregistering module %s\n", info->name); - AST_LIST_HEAD_DESTROY(&mod->users); - ao2_cleanup(mod->ref_debug); - ast_free(mod); + module_destroy(mod); } } @@ -511,6 +529,8 @@ static struct ast_module *load_dynamic_module(const char *resource_in, unsigned int space; /* room needed for the descriptor */ int missing_so = 0; + ast_assert(!resource_being_loaded); + space = sizeof(*resource_being_loaded) + strlen(resource_in) + 1; if (strcasecmp(resource_in + strlen(resource_in) - 3, ".so")) { missing_so = 1; @@ -523,34 +543,23 @@ static struct ast_module *load_dynamic_module(const char *resource_in, unsigned any symbols, and don't export any symbols. this will allow us to peek into the module's info block (if available) to see what flags it has set */ - resource_being_loaded = ast_calloc(1, space); + mod = resource_being_loaded = ast_calloc(1, space); if (!resource_being_loaded) return NULL; strcpy(resource_being_loaded->resource, resource_in); if (missing_so) strcat(resource_being_loaded->resource, ".so"); - if (!(lib = dlopen(fn, RTLD_LAZY | RTLD_GLOBAL))) { - if (!suppress_logging) { + lib = dlopen(fn, RTLD_LAZY | RTLD_GLOBAL); + if (resource_being_loaded) { + resource_being_loaded = NULL; + if (lib) { + ast_log(LOG_ERROR, "Module '%s' did not register itself during load\n", resource_in); + logged_dlclose(resource_in, lib); + } else if (!suppress_logging) { ast_log(LOG_WARNING, "Error loading module '%s': %s\n", resource_in, dlerror()); } - ast_free(resource_being_loaded); - return NULL; - } - - /* the dlopen() succeeded, let's find out if the module - registered itself */ - /* note that this will only work properly as long as - ast_module_register() (which is called by the module's - constructor) places the new module at the tail of the - module_list - */ - if (resource_being_loaded != (mod = AST_DLLIST_LAST(&module_list))) { - ast_log(LOG_WARNING, "Module '%s' did not register itself during load\n", resource_in); - /* no, it did not, so close it and return */ - logged_dlclose(resource_in, lib); - /* note that the module's destructor will call ast_module_unregister(), - which will free the structure we allocated in resource_being_loaded */ + ast_free(mod); return NULL; } @@ -564,19 +573,20 @@ static struct ast_module *load_dynamic_module(const char *resource_in, unsigned } logged_dlclose(resource_in, lib); - resource_being_loaded = NULL; /* start the load process again */ - resource_being_loaded = ast_calloc(1, space); + mod = resource_being_loaded = ast_calloc(1, space); if (!resource_being_loaded) return NULL; strcpy(resource_being_loaded->resource, resource_in); if (missing_so) strcat(resource_being_loaded->resource, ".so"); - if (!(lib = dlopen(fn, wants_global ? RTLD_LAZY | RTLD_GLOBAL : RTLD_NOW | RTLD_LOCAL))) { + lib = dlopen(fn, wants_global ? RTLD_LAZY | RTLD_GLOBAL : RTLD_NOW | RTLD_LOCAL); + resource_being_loaded = NULL; + if (!lib) { ast_log(LOG_WARNING, "Error loading module '%s': %s\n", resource_in, dlerror()); - ast_free(resource_being_loaded); + ast_free(mod); return NULL; } @@ -585,7 +595,6 @@ static struct ast_module *load_dynamic_module(const char *resource_in, unsigned time too :) */ AST_DLLIST_LAST(&module_list)->lib = lib; - resource_being_loaded = NULL; return AST_DLLIST_LAST(&module_list); } @@ -621,9 +630,7 @@ int modules_shutdown(void) ast_verb(1, "Unloading %s\n", mod->resource); mod->info->unload(); } - AST_LIST_HEAD_DESTROY(&mod->users); - ao2_cleanup(mod->ref_debug); - ast_free(mod); + module_destroy(mod); somethingchanged = 1; } AST_DLLIST_TRAVERSE_BACKWARDS_SAFE_END; -- cgit v1.2.3