[Asterisk-code-review] loader: Minor fix to module registration. (asterisk[13])

Corey Farrell asteriskteam at digium.com
Fri Dec 15 09:44:57 CST 2017


Corey Farrell has uploaded this change for review. ( https://gerrit.asterisk.org/7603


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, 59 insertions(+), 54 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/03/7603/1

diff --git a/main/loader.c b/main/loader.c
index b1a02d5..0cb168f 100644
--- a/main/loader.c
+++ b/main/loader.c
@@ -171,19 +171,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;
 #ifdef REF_DEBUG
@@ -191,23 +216,20 @@
 #endif
 	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);
+#ifdef REF_DEBUG
+	ao2_cleanup(mod->ref_debug);
+#endif
+	ast_free(mod);
 }
 
 void ast_module_unregister(const struct ast_module_info *info)
@@ -230,11 +252,7 @@
 
 	if (mod) {
 		ast_debug(5, "Unregistering module %s\n", info->name);
-		AST_LIST_HEAD_DESTROY(&mod->users);
-#ifdef REF_DEBUG
-		ao2_cleanup(mod->ref_debug);
-#endif
-		ast_free(mod);
+		module_destroy(mod);
 	}
 }
 
@@ -517,6 +535,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;
@@ -529,34 +549,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;
 	}
 
@@ -570,19 +579,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;
 	}
 
@@ -591,7 +601,6 @@
 	   time too :) */
 
 	AST_DLLIST_LAST(&module_list)->lib = lib;
-	resource_being_loaded = NULL;
 
 	return AST_DLLIST_LAST(&module_list);
 }
@@ -627,11 +636,7 @@
 				ast_verb(1, "Unloading %s\n", mod->resource);
 				mod->info->unload();
 			}
-			AST_LIST_HEAD_DESTROY(&mod->users);
-#ifdef REF_DEBUG
-			ao2_cleanup(mod->ref_debug);
-#endif
-			free(mod);
+			module_destroy(mod);
 			somethingchanged = 1;
 		}
 		AST_DLLIST_TRAVERSE_BACKWARDS_SAFE_END;

-- 
To view, visit https://gerrit.asterisk.org/7603
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: newchange
Gerrit-Change-Id: I40f07a315e55b92df4fc7faf525ed6d4f396e7d2
Gerrit-Change-Number: 7603
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/7c25a94e/attachment-0001.html>


More information about the asterisk-code-review mailing list