[Asterisk-code-review] loader: Minor fix to module registration. (asterisk[15])
Corey Farrell
asteriskteam at digium.com
Fri Dec 15 09:40:39 CST 2017
Corey Farrell has uploaded this change for review. ( https://gerrit.asterisk.org/7599
Change subject: loader: Minor fix to module registration.
......................................................................
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
---
M main/loader.c
1 file changed, 57 insertions(+), 50 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/99/7599/1
diff --git a/main/loader.c b/main/loader.c
index 23a29d9..f06d637 100644
--- a/main/loader.c
+++ b/main/loader.c
@@ -167,19 +167,44 @@
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) {
@@ -187,23 +212,18 @@
}
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);
/* give the module a copy of its own handle, for later use in registrations and the like */
*((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)
@@ -226,9 +246,7 @@
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 @@
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 @@
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 @@
}
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 @@
time too :) */
AST_DLLIST_LAST(&module_list)->lib = lib;
- resource_being_loaded = NULL;
return AST_DLLIST_LAST(&module_list);
}
@@ -621,9 +630,7 @@
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;
--
To view, visit https://gerrit.asterisk.org/7599
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: newchange
Gerrit-Change-Id: I40f07a315e55b92df4fc7faf525ed6d4f396e7d2
Gerrit-Change-Number: 7599
Gerrit-PatchSet: 1
Gerrit-Owner: Corey Farrell <git at cfware.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171215/ab9783db/attachment-0001.html>
More information about the asterisk-code-review
mailing list