<p>Corey Farrell has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.asterisk.org/7599">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">loader: Minor fix to module registration.<br><br>This protects the module loader itself against crashing if dlopen is<br>called on a module from outside loader.c.<br><br>* Expand scope of lock inside ast_module_register to include reading of<br>  resource_being_loaded.<br>* NULL check resource_being_loaded.<br>* Set resource_being_loaded NULL as soon as dlopen returns.  This fixes<br>  some error paths where it was not NULL'ed.<br>* Create module_destroy function to deduplicate code from<br>  ast_module_unregister and modules_shutdown.<br>* Resolve leak that occured if a module did not successfully register.<br>* Simplify checking for successful registration.<br><br>Change-Id: I40f07a315e55b92df4fc7faf525ed6d4f396e7d2<br>---<br>M main/loader.c<br>1 file changed, 57 insertions(+), 50 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/99/7599/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">diff --git a/main/loader.c b/main/loader.c<br>index 23a29d9..f06d637 100644<br>--- a/main/loader.c<br>+++ b/main/loader.c<br>@@ -167,19 +167,44 @@<br> <br> static AST_DLLIST_HEAD_STATIC(reload_queue, reload_queue_item);<br> <br>-/* when dynamic modules are being loaded, ast_module_register() will<br>-   need to know what filename the module was loaded from while it<br>-   is being registered<br>-*/<br>+/*!<br>+ * \internal<br>+ *<br>+ * This variable is set by load_dynamic_module so ast_module_register<br>+ * can know what pointer is being registered.<br>+ *<br>+ * This is protected by the module_list lock.<br>+ */<br> static struct ast_module *resource_being_loaded;<br> <br>-/* XXX: should we check for duplicate resource names here? */<br>-<br>+/*!<br>+ * \internal<br>+ * \brief Used by AST_MODULE_INFO to register with the module loader.<br>+ *<br>+ * This function is automatically called when each module is opened.<br>+ * It must never be used from outside AST_MODULE_INFO.<br>+ */<br> void ast_module_register(const struct ast_module_info *info)<br> {<br>-        struct ast_module *mod = resource_being_loaded;<br>+      struct ast_module *mod;<br>+<br>+   /*<br>+    * This lock protects resource_being_loaded as well as the module<br>+     * list.  Normally we already have a lock on module_list when we<br>+      * begin the load but locking again from here prevents corruption<br>+     * if an asterisk module is dlopen'ed from outside the module loader.<br>+     */<br>+  AST_DLLIST_LOCK(&module_list);<br>+   mod = resource_being_loaded;<br>+ if (!mod) {<br>+          AST_DLLIST_UNLOCK(&module_list);<br>+         return;<br>+      }<br> <br>  ast_debug(5, "Registering module %s\n", info->name);<br>+<br>+ /* This tells load_dynamic_module that we're registered. */<br>+      resource_being_loaded = NULL;<br> <br>      mod->info = info;<br>  if (ast_opt_ref_debug) {<br>@@ -187,23 +212,18 @@<br>       }<br>     AST_LIST_HEAD_INIT(&mod->users);<br> <br>-   /* during startup, before the loader has been initialized,<br>-      there are no threads, so there is no need to take the lock<br>-           on this list to manipulate it. it is also possible that it<br>-           might be unsafe to use the list lock at that point... so<br>-     let's avoid it altogether<br>-     */<br>-   AST_DLLIST_LOCK(&module_list);<br>-   /* it is paramount that the new entry be placed at the tail of<br>-          the list, otherwise the code that uses dlopen() to load<br>-      dynamic modules won't be able to find out if the module it<br>-       just opened was registered or failed to load<br>-      */<br>    AST_DLLIST_INSERT_TAIL(&module_list, mod, entry);<br>         AST_DLLIST_UNLOCK(&module_list);<br> <br>       /* give the module a copy of its own handle, for later use in registrations and the like */<br>   *((struct ast_module **) &(info->self)) = mod;<br>+}<br>+<br>+static void module_destroy(struct ast_module *mod)<br>+{<br>+        AST_LIST_HEAD_DESTROY(&mod->users);<br>+   ao2_cleanup(mod->ref_debug);<br>+      ast_free(mod);<br> }<br> <br> void ast_module_unregister(const struct ast_module_info *info)<br>@@ -226,9 +246,7 @@<br> <br>        if (mod) {<br>            ast_debug(5, "Unregistering module %s\n", info->name);<br>-          AST_LIST_HEAD_DESTROY(&mod->users);<br>-           ao2_cleanup(mod->ref_debug);<br>-              ast_free(mod);<br>+               module_destroy(mod);<br>  }<br> }<br> <br>@@ -511,6 +529,8 @@<br>         int space;      /* room needed for the descriptor */<br>  int missing_so = 0;<br> <br>+       ast_assert(!resource_being_loaded);<br>+<br>        space = sizeof(*resource_being_loaded) + strlen(resource_in) + 1;<br>     if (strcasecmp(resource_in + strlen(resource_in) - 3, ".so")) {<br>             missing_so = 1;<br>@@ -523,34 +543,23 @@<br>           any symbols, and don't export any symbols. this will allow us to peek into<br>        the module's info block (if available) to see what flags it has set */<br> <br>-     resource_being_loaded = ast_calloc(1, space);<br>+        mod = resource_being_loaded = ast_calloc(1, space);<br>   if (!resource_being_loaded)<br>           return NULL;<br>  strcpy(resource_being_loaded->resource, resource_in);<br>      if (missing_so)<br>               strcat(resource_being_loaded->resource, ".so");<br> <br>-      if (!(lib = dlopen(fn, RTLD_LAZY | RTLD_GLOBAL))) {<br>-          if (!suppress_logging) {<br>+     lib = dlopen(fn, RTLD_LAZY | RTLD_GLOBAL);<br>+   if (resource_being_loaded) {<br>+         resource_being_loaded = NULL;<br>+                if (lib) {<br>+                   ast_log(LOG_ERROR, "Module '%s' did not register itself during load\n", resource_in);<br>+                      logged_dlclose(resource_in, lib);<br>+            } else if (!suppress_logging) {<br>                       ast_log(LOG_WARNING, "Error loading module '%s': %s\n", resource_in, dlerror());<br>            }<br>-            ast_free(resource_being_loaded);<br>-             return NULL;<br>- }<br>-<br>- /* the dlopen() succeeded, let's find out if the module<br>-     registered itself */<br>-      /* note that this will only work properly as long as<br>-    ast_module_register() (which is called by the module's<br>-           constructor) places the new module at the tail of the<br>-        module_list<br>-       */<br>-   if (resource_being_loaded != (mod = AST_DLLIST_LAST(&module_list))) {<br>-            ast_log(LOG_WARNING, "Module '%s' did not register itself during load\n", resource_in);<br>-            /* no, it did not, so close it and return */<br>-         logged_dlclose(resource_in, lib);<br>-            /* note that the module's destructor will call ast_module_unregister(),<br>-             which will free the structure we allocated in resource_being_loaded */<br>+            ast_free(mod);<br>                return NULL;<br>  }<br> <br>@@ -564,19 +573,20 @@<br>   }<br> <br>  logged_dlclose(resource_in, lib);<br>-    resource_being_loaded = NULL;<br> <br>      /* start the load process again */<br>-   resource_being_loaded = ast_calloc(1, space);<br>+        mod = resource_being_loaded = ast_calloc(1, space);<br>   if (!resource_being_loaded)<br>           return NULL;<br>  strcpy(resource_being_loaded->resource, resource_in);<br>      if (missing_so)<br>               strcat(resource_being_loaded->resource, ".so");<br> <br>-      if (!(lib = dlopen(fn, wants_global ? RTLD_LAZY | RTLD_GLOBAL : RTLD_NOW | RTLD_LOCAL))) {<br>+   lib = dlopen(fn, wants_global ? RTLD_LAZY | RTLD_GLOBAL : RTLD_NOW | RTLD_LOCAL);<br>+    resource_being_loaded = NULL;<br>+        if (!lib) {<br>           ast_log(LOG_WARNING, "Error loading module '%s': %s\n", resource_in, dlerror());<br>-           ast_free(resource_being_loaded);<br>+             ast_free(mod);<br>                return NULL;<br>  }<br> <br>@@ -585,7 +595,6 @@<br>        time too :) */<br> <br>  AST_DLLIST_LAST(&module_list)->lib = lib;<br>-     resource_being_loaded = NULL;<br> <br>      return AST_DLLIST_LAST(&module_list);<br> }<br>@@ -621,9 +630,7 @@<br>                            ast_verb(1, "Unloading %s\n", mod->resource);<br>                            mod->info->unload();<br>                    }<br>-                    AST_LIST_HEAD_DESTROY(&mod->users);<br>-                   ao2_cleanup(mod->ref_debug);<br>-                      ast_free(mod);<br>+                       module_destroy(mod);<br>                  somethingchanged = 1;<br>                 }<br>             AST_DLLIST_TRAVERSE_BACKWARDS_SAFE_END;<br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/7599">change 7599</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/7599"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15 </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>
<div style="display:none"> Gerrit-Change-Id: I40f07a315e55b92df4fc7faf525ed6d4f396e7d2 </div>
<div style="display:none"> Gerrit-Change-Number: 7599 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Corey Farrell <git@cfware.com> </div>