<p>Joshua Colp <strong>merged</strong> this change.</p><p><a href="https://gerrit.asterisk.org/7770">View Change</a></p><div style="white-space:pre-wrap">Approvals:
Kevin Harwell: Looks good to me, but someone else must approve
Joshua Colp: Looks good to me, approved; Approved for Submit
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">loader: Create ast_module_running_ref.<br><br>This function returns NULL if the module in question is not running. I<br>did not change ast_module_ref as most callers do not check the result<br>and they always call ast_module_unref.<br><br>Make use of this function when running registered items from:<br>* app_stack API's<br>* bridge technologies<br>* CLI commands<br>* File formats<br>* Manager Actions<br>* RTP engines<br>* Sorcery Wizards<br>* Timing Interfaces<br>* Translators<br>* AGI Commands<br>* Fax Technologies<br><br>ASTERISK-20346 #close<br><br>Change-Id: Ia16fd28e188b2fc0b9d18b8a5d9cacc31df73fcc<br>---<br>M include/asterisk/module.h<br>M main/app.c<br>M main/bridge.c<br>M main/cli.c<br>M main/file.c<br>M main/loader.c<br>M main/manager.c<br>M main/rtp_engine.c<br>M main/sorcery.c<br>M main/timing.c<br>M main/translate.c<br>M res/res_agi.c<br>M res/res_fax.c<br>13 files changed, 161 insertions(+), 80 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">diff --git a/include/asterisk/module.h b/include/asterisk/module.h<br>index d3b9ddc..103cd30 100644<br>--- a/include/asterisk/module.h<br>+++ b/include/asterisk/module.h<br>@@ -357,6 +357,7 @@<br> #define ast_module_user_hangup_all() __ast_module_user_hangup_all(AST_MODULE_SELF)<br> <br> struct ast_module *__ast_module_ref(struct ast_module *mod, const char *file, int line, const char *func);<br>+struct ast_module *__ast_module_running_ref(struct ast_module *mod, const char *file, int line, const char *func);<br> void __ast_module_shutdown_ref(struct ast_module *mod, const char *file, int line, const char *func);<br> void __ast_module_unref(struct ast_module *mod, const char *file, int line, const char *func);<br> <br>@@ -369,6 +370,20 @@<br> * from being unloaded.<br> */<br> #define ast_module_ref(mod) __ast_module_ref(mod, __FILE__, __LINE__, __PRETTY_FUNCTION__)<br>+<br>+/*!<br>+ * \brief Hold a reference to the module if it is running.<br>+ * \param mod Module to reference<br>+ * \retval mod if running<br>+ * \retval NULL if not running<br>+ *<br>+ * The returned pointer should be released with ast_module_unref.<br>+ *<br>+ * \note A module reference will prevent the module from being unloaded.<br>+ */<br>+#define ast_module_running_ref(mod) \<br>+ __ast_module_running_ref(mod, __FILE__, __LINE__, __PRETTY_FUNCTION__)<br>+<br> /*!<br> * \brief Prevent unload of the module before shutdown<br> * \param mod Module to hold<br>diff --git a/main/app.c b/main/app.c<br>index 215ec04..2db9a8f 100644<br>--- a/main/app.c<br>+++ b/main/app.c<br>@@ -386,6 +386,7 @@<br> return res;<br> }<br> <br>+/* BUGBUG this is not thread safe. */<br> static const struct ast_app_stack_funcs *app_stack_callbacks;<br> <br> void ast_install_stack_functions(const struct ast_app_stack_funcs *funcs)<br>@@ -399,16 +400,16 @@<br> const char *new_args;<br> <br> funcs = app_stack_callbacks;<br>- if (!funcs || !funcs->expand_sub_args) {<br>+ if (!funcs || !funcs->expand_sub_args || !ast_module_running_ref(funcs->module)) {<br> ast_log(LOG_WARNING,<br> "Cannot expand 'Gosub(%s)' arguments. The app_stack module is not available.\n",<br> args);<br> return NULL;<br> }<br>- ast_module_ref(funcs->module);<br> <br> new_args = funcs->expand_sub_args(chan, args);<br> ast_module_unref(funcs->module);<br>+<br> return new_args;<br> }<br> <br>@@ -418,13 +419,12 @@<br> int res;<br> <br> funcs = app_stack_callbacks;<br>- if (!funcs || !funcs->run_sub) {<br>+ if (!funcs || !funcs->run_sub || !ast_module_running_ref(funcs->module)) {<br> ast_log(LOG_WARNING,<br> "Cannot run 'Gosub(%s)'. The app_stack module is not available.\n",<br> sub_args);<br> return -1;<br> }<br>- ast_module_ref(funcs->module);<br> <br> if (autoservice_chan) {<br> ast_autoservice_start(autoservice_chan);<br>diff --git a/main/bridge.c b/main/bridge.c<br>index 88d9e54..4f79852 100644<br>--- a/main/bridge.c<br>+++ b/main/bridge.c<br>@@ -234,8 +234,21 @@<br> /* Copy module pointer so reference counting can keep the module from unloading */<br> technology->mod = module;<br> <br>- /* Insert our new bridge technology into the list and print out a pretty message */<br>- AST_RWLIST_INSERT_TAIL(&bridge_technologies, technology, entry);<br>+ /* Find the correct position to insert the technology. */<br>+ AST_RWLIST_TRAVERSE_SAFE_BEGIN(&bridge_technologies, current, entry) {<br>+ /* Put the highest preference tech's first in the list. */<br>+ if (technology->preference >= current->preference) {<br>+ AST_RWLIST_INSERT_BEFORE_CURRENT(technology, entry);<br>+<br>+ break;<br>+ }<br>+ }<br>+ AST_RWLIST_TRAVERSE_SAFE_END;<br>+<br>+ if (!current) {<br>+ /* Insert our new bridge technology to the end of the list. */<br>+ AST_RWLIST_INSERT_TAIL(&bridge_technologies, technology, entry);<br>+ }<br> <br> AST_RWLIST_UNLOCK(&bridge_technologies);<br> <br>@@ -517,12 +530,17 @@<br> current->name);<br> continue;<br> }<br>+ if (!ast_module_running_ref(current->mod)) {<br>+ ast_debug(1, "Bridge technology %s is not running, skipping.\n", current->name);<br>+ continue;<br>+ }<br>+ if (best) {<br>+ ast_module_unref(best->mod);<br>+ }<br> best = current;<br> }<br> <br> if (best) {<br>- /* Increment it's module reference count if present so it does not get unloaded while in use */<br>- ast_module_ref(best->mod);<br> ast_debug(1, "Chose bridge technology %s\n", best->name);<br> }<br> <br>diff --git a/main/cli.c b/main/cli.c<br>index 1299c2a..80c1843 100644<br>--- a/main/cli.c<br>+++ b/main/cli.c<br>@@ -2679,9 +2679,12 @@<br> .n = state - matchnum,<br> .argv = argv,<br> .argc = x};<br>- ast_module_ref(e->module);<br>- ret = e->handler(e, CLI_GENERATE, &a);<br>- ast_module_unref(e->module);<br>+<br>+ /* If the command is in a module it must be running. */<br>+ if (!e->module || ast_module_running_ref(e->module)) {<br>+ ret = e->handler(e, CLI_GENERATE, &a);<br>+ ast_module_unref(e->module);<br>+ }<br> }<br> if (ret)<br> break;<br>@@ -2760,9 +2763,11 @@<br> */<br> args[0] = (char *)e;<br> <br>- ast_module_ref(e->module);<br>- retval = e->handler(e, CLI_HANDLER, &a);<br>- ast_module_unref(e->module);<br>+ /* If the command is in a module it must be running. */<br>+ if (!e->module || ast_module_running_ref(e->module)) {<br>+ retval = e->handler(e, CLI_HANDLER, &a);<br>+ ast_module_unref(e->module);<br>+ }<br> <br> if (retval == CLI_SHOWUSAGE) {<br> ast_cli(fd, "%s", S_OR(e->usage, "Invalid usage, but no usage information available.\n"));<br>diff --git a/main/file.c b/main/file.c<br>index 41131f9..8dded81 100644<br>--- a/main/file.c<br>+++ b/main/file.c<br>@@ -425,11 +425,17 @@<br> static struct ast_filestream *get_filestream(struct ast_format_def *fmt, FILE *bfile)<br> {<br> struct ast_filestream *s;<br>-<br> int l = sizeof(*s) + fmt->buf_size + fmt->desc_size; /* total allocation size */<br>- if ( (s = ao2_alloc(l, filestream_destructor)) == NULL)<br>+<br>+ if (!ast_module_running_ref(fmt->module)) {<br> return NULL;<br>- ast_module_ref(fmt->module);<br>+ }<br>+<br>+ s = ao2_alloc(l, filestream_destructor);<br>+ if (!s) {<br>+ ast_module_unref(fmt->module);<br>+ return NULL;<br>+ }<br> s->fmt = fmt;<br> s->f = bfile;<br> <br>diff --git a/main/loader.c b/main/loader.c<br>index fe18048..88c1cda 100644<br>--- a/main/loader.c<br>+++ b/main/loader.c<br>@@ -1702,6 +1702,16 @@<br> return mod;<br> }<br> <br>+struct ast_module *__ast_module_running_ref(struct ast_module *mod,<br>+ const char *file, int line, const char *func)<br>+{<br>+ if (!mod || !mod->flags.running) {<br>+ return NULL;<br>+ }<br>+<br>+ return __ast_module_ref(mod, file, line, func);<br>+}<br>+<br> void __ast_module_shutdown_ref(struct ast_module *mod, const char *file, int line, const char *func)<br> {<br> if (!mod || mod->flags.keepuntilshutdown) {<br>diff --git a/main/manager.c b/main/manager.c<br>index 576978c..3aa9105 100644<br>--- a/main/manager.c<br>+++ b/main/manager.c<br>@@ -2923,34 +2923,41 @@<br> }<br> <br> action = astman_get_header(&m, "Action");<br>- if (strcasecmp(action, "login")) {<br>- act_found = action_find(action);<br>- if (act_found) {<br>- /*<br>- * we have to simulate a session for this action request<br>- * to be able to pass it down for processing<br>- * This is necessary to meet the previous design of manager.c<br>- */<br>- s.hook = hook;<br> <br>- ao2_lock(act_found);<br>- if (act_found->registered && act_found->func) {<br>- if (act_found->module) {<br>- ast_module_ref(act_found->module);<br>- }<br>- ao2_unlock(act_found);<br>- ret = act_found->func(&s, &m);<br>- ao2_lock(act_found);<br>- if (act_found->module) {<br>- ast_module_unref(act_found->module);<br>- }<br>- } else {<br>- ret = -1;<br>- }<br>- ao2_unlock(act_found);<br>- ao2_t_ref(act_found, -1, "done with found action object");<br>+ do {<br>+ if (!strcasecmp(action, "login")) {<br>+ break;<br> }<br>- }<br>+<br>+ act_found = action_find(action);<br>+ if (!act_found) {<br>+ break;<br>+ }<br>+<br>+ /*<br>+ * we have to simulate a session for this action request<br>+ * to be able to pass it down for processing<br>+ * This is necessary to meet the previous design of manager.c<br>+ */<br>+ s.hook = hook;<br>+<br>+ ret = -1;<br>+ ao2_lock(act_found);<br>+ if (act_found->registered && act_found->func) {<br>+ struct ast_module *mod_ref = ast_module_running_ref(act_found->module);<br>+<br>+ ao2_unlock(act_found);<br>+ /* If the action is in a module it must be running. */<br>+ if (!act_found->module || mod_ref) {<br>+ ret = act_found->func(&s, &m);<br>+ ast_module_unref(mod_ref);<br>+ }<br>+ } else {<br>+ ao2_unlock(act_found);<br>+ }<br>+ ao2_t_ref(act_found, -1, "done with found action object");<br>+ } while (0);<br>+<br> ast_free(dup_str);<br> return ret;<br> }<br>@@ -6442,21 +6449,21 @@<br> if ((s->session->writeperm & act_found->authority)<br> || act_found->authority == 0) {<br> /* We have the authority to execute the action. */<br>+ ret = -1;<br> ao2_lock(act_found);<br> if (act_found->registered && act_found->func) {<br>- ast_debug(1, "Running action '%s'\n", act_found->action);<br>- if (act_found->module) {<br>- ast_module_ref(act_found->module);<br>- }<br>+ struct ast_module *mod_ref = ast_module_running_ref(act_found->module);<br>+<br> ao2_unlock(act_found);<br>- ret = act_found->func(s, m);<br>- acted = 1;<br>- ao2_lock(act_found);<br>- if (act_found->module) {<br>- ast_module_unref(act_found->module);<br>+ if (mod_ref || !act_found->module) {<br>+ ast_debug(1, "Running action '%s'\n", act_found->action);<br>+ ret = act_found->func(s, m);<br>+ acted = 1;<br>+ ast_module_unref(mod_ref);<br> }<br>+ } else {<br>+ ao2_unlock(act_found);<br> }<br>- ao2_unlock(act_found);<br> }<br> if (!acted) {<br> /*<br>diff --git a/main/rtp_engine.c b/main/rtp_engine.c<br>index 76bdf87..1c73477 100644<br>--- a/main/rtp_engine.c<br>+++ b/main/rtp_engine.c<br>@@ -428,6 +428,7 @@<br> struct ast_sockaddr address = {{0,}};<br> struct ast_rtp_instance *instance = NULL;<br> struct ast_rtp_engine *engine = NULL;<br>+ struct ast_module *mod_ref;<br> <br> AST_RWLIST_RDLOCK(&engines);<br> <br>@@ -450,10 +451,15 @@<br> }<br> <br> /* Bump up the reference count before we return so the module can not be unloaded */<br>- ast_module_ref(engine->mod);<br>+ mod_ref = ast_module_running_ref(engine->mod);<br> <br> AST_RWLIST_UNLOCK(&engines);<br> <br>+ if (!mod_ref) {<br>+ /* BUGBUG: improve handling of this situation. */<br>+ return NULL;<br>+ }<br>+<br> /* Allocate a new RTP instance */<br> if (!(instance = ao2_alloc(sizeof(*instance), instance_destructor))) {<br> ast_module_unref(engine->mod);<br>diff --git a/main/sorcery.c b/main/sorcery.c<br>index a55d5c7..c79675c 100644<br>--- a/main/sorcery.c<br>+++ b/main/sorcery.c<br>@@ -835,16 +835,19 @@<br> RAII_VAR(struct ast_sorcery_object_wizard *, object_wizard, ao2_alloc(sizeof(*object_wizard), sorcery_object_wizard_destructor), ao2_cleanup);<br> int created = 0;<br> <br>- if (!wizard) {<br>+ if (!object_wizard) {<br>+ return AST_SORCERY_APPLY_FAIL;<br>+ }<br>+<br>+ if (!wizard || wizard->callbacks.module != ast_module_running_ref(wizard->callbacks.module)) {<br> ast_log(LOG_ERROR, "Wizard '%s' could not be applied to object type '%s' as it was not found\n",<br> name, type);<br>- return AST_SORCERY_APPLY_FAIL;<br>- } else if (!object_wizard) {<br> return AST_SORCERY_APPLY_FAIL;<br> }<br> <br> if (!object_type) {<br> if (!(object_type = sorcery_object_type_alloc(type, module))) {<br>+ ast_module_unref(wizard->callbacks.module);<br> return AST_SORCERY_APPLY_FAIL;<br> }<br> created = 1;<br>@@ -861,6 +864,7 @@<br> ast_debug(1, "Wizard %s already applied to object type %s\n",<br> wizard->callbacks.name, object_type->name);<br> AST_VECTOR_RW_UNLOCK(&object_type->wizards);<br>+ ast_module_unref(wizard->callbacks.module);<br> return AST_SORCERY_APPLY_DUPLICATE;<br> }<br> }<br>@@ -871,10 +875,9 @@<br> ast_log(LOG_WARNING, "Wizard '%s' failed to open mapping for object type '%s' with data: %s\n",<br> name, object_type->name, S_OR(data, ""));<br> AST_VECTOR_RW_UNLOCK(&object_type->wizards);<br>+ ast_module_unref(wizard->callbacks.module);<br> return AST_SORCERY_APPLY_FAIL;<br> }<br>-<br>- ast_module_ref(wizard->callbacks.module);<br> <br> object_wizard->wizard = ao2_bump(wizard);<br> object_wizard->caching = caching;<br>diff --git a/main/timing.c b/main/timing.c<br>index c6a9480..18852c6 100644<br>--- a/main/timing.c<br>+++ b/main/timing.c<br>@@ -124,17 +124,22 @@<br> void *data = NULL;<br> struct timing_holder *h;<br> struct ast_timer *t = NULL;<br>+ int idx = 1;<br> <br> ast_heap_rdlock(timing_interfaces);<br> <br>- if ((h = ast_heap_peek(timing_interfaces, 1))) {<br>- data = h->iface->timer_open();<br>- ast_module_ref(h->mod);<br>+ while ((h = ast_heap_peek(timing_interfaces, idx))) {<br>+ if (ast_module_running_ref(h->mod)) {<br>+ data = h->iface->timer_open();<br>+ break;<br>+ }<br>+ idx++;<br> }<br> <br> if (data) {<br> if (!(t = ast_calloc(1, sizeof(*t)))) {<br> h->iface->timer_close(data);<br>+ ast_module_unref(h->mod);<br> } else {<br> t->data = data;<br> t->holder = h;<br>diff --git a/main/translate.c b/main/translate.c<br>index 02717c5..3d49057 100644<br>--- a/main/translate.c<br>+++ b/main/translate.c<br>@@ -342,7 +342,10 @@<br> */<br> pvt->explicit_dst = ao2_bump(explicit_dst);<br> <br>- ast_module_ref(t->module);<br>+ if (!ast_module_running_ref(t->module)) {<br>+ ast_free(pvt);<br>+ return NULL;<br>+ }<br> <br> /* call local init routine, if present */<br> if (t->newpvt && t->newpvt(pvt)) {<br>diff --git a/res/res_agi.c b/res/res_agi.c<br>index 85914c0..393a503 100644<br>--- a/res/res_agi.c<br>+++ b/res/res_agi.c<br>@@ -4040,14 +4040,19 @@<br> <br> parse_args(buf, &argc, argv);<br> c = find_command(argv, 0);<br>- if (c && (!dead || (dead && c->dead))) {<br>- /* if this command wasn't registered by res_agi, be sure to usecount<br>- the module we are using */<br>- if (c->mod != ast_module_info->self)<br>- ast_module_ref(c->mod);<br>+ if (!c || !ast_module_running_ref(c->mod)) {<br>+ ami_res = "Invalid or unknown command";<br>+ resultcode = 510;<br>+<br>+ ast_agi_send(agi->fd, chan, "%d %s\n", resultcode, ami_res);<br>+<br>+ publish_async_exec_end(chan, command_id, ami_cmd, resultcode, ami_res);<br>+<br>+ return AGI_RESULT_SUCCESS;<br>+ }<br>+<br>+ if (!dead || (dead && c->dead)) {<br> res = c->handler(chan, agi, argc, argv);<br>- if (c->mod != ast_module_info->self)<br>- ast_module_unref(c->mod);<br> switch (res) {<br> case RESULT_SHOWUSAGE:<br> ami_res = "Usage";<br>@@ -4094,21 +4099,15 @@<br> <br> break;<br> }<br>- } else if (c) {<br>+ } else {<br> ami_res = "Command Not Permitted on a dead channel or intercept routine";<br> resultcode = 511;<br> <br> ast_agi_send(agi->fd, chan, "%d %s\n", resultcode, ami_res);<br> <br> publish_async_exec_end(chan, command_id, ami_cmd, resultcode, ami_res);<br>- } else {<br>- ami_res = "Invalid or unknown command";<br>- resultcode = 510;<br>-<br>- ast_agi_send(agi->fd, chan, "%d %s\n", resultcode, ami_res);<br>-<br>- publish_async_exec_end(chan, command_id, ami_cmd, resultcode, ami_res);<br> }<br>+ ast_module_unref(c->mod);<br> <br> return AGI_RESULT_SUCCESS;<br> }<br>diff --git a/res/res_fax.c b/res/res_fax.c<br>index 0938452..4be5aee 100644<br>--- a/res/res_fax.c<br>+++ b/res/res_fax.c<br>@@ -1161,8 +1161,10 @@<br> if ((faxmod->tech->caps & details->caps) != details->caps) {<br> continue;<br> }<br>+ if (!ast_module_running_ref(faxmod->tech->module)) {<br>+ continue;<br>+ }<br> ast_debug(4, "Reserving a FAX session from '%s'.\n", faxmod->tech->description);<br>- ast_module_ref(faxmod->tech->module);<br> s->tech = faxmod->tech;<br> break;<br> }<br>@@ -1279,8 +1281,10 @@<br> if ((faxmod->tech->caps & details->caps) != details->caps) {<br> continue;<br> }<br>+ if (!ast_module_running_ref(faxmod->tech->module)) {<br>+ continue;<br>+ }<br> ast_debug(4, "Requesting a new FAX session from '%s'.\n", faxmod->tech->description);<br>- ast_module_ref(faxmod->tech->module);<br> if (reserved) {<br> /* Balance module ref from reserved session */<br> ast_module_unref(reserved->tech->module);<br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/7770">change 7770</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/7770"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: merged </div>
<div style="display:none"> Gerrit-Change-Id: Ia16fd28e188b2fc0b9d18b8a5d9cacc31df73fcc </div>
<div style="display:none"> Gerrit-Change-Number: 7770 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>