Combined patch from upstream (All authored by myself): https://gerrit.asterisk.org/c/asterisk/+/15942 (CLI: locks show) https://gerrit.asterisk.org/c/asterisk/+/15943 (unload memory corruption) https://gerrit.asterisk.org/c/asterisk/+/15944 (error path ref counting) https://gerrit.asterisk.org/c/asterisk/+/15945 (ast_module_ref usage) The cause of my nightmares was the unload memory corruption, however, the other two whilst much less likely to occur are just as serious. Fixes on all has been well tested. The individual patches are quite small. Signed-off-by: Jaco Kroon --- diff --git a/funcs/func_lock.c b/funcs/func_lock.c index 072640751e..31a7fcda29 100644 --- a/funcs/func_lock.c +++ b/funcs/func_lock.c @@ -42,6 +42,7 @@ #include "asterisk/linkedlists.h" #include "asterisk/astobj2.h" #include "asterisk/utils.h" +#include "asterisk/cli.h" /*** DOCUMENTATION @@ -157,6 +158,8 @@ static void lock_free(void *data) AST_LIST_UNLOCK(oldlist); AST_LIST_HEAD_DESTROY(oldlist); ast_free(oldlist); + + ast_module_unref(ast_module_info->self); } static void lock_fixup(void *data, struct ast_channel *oldchan, struct ast_channel *newchan) @@ -191,7 +194,12 @@ static int get_lock(struct ast_channel *chan, char *lockname, int trylock) struct timeval now; if (!lock_store) { - ast_debug(1, "Channel %s has no lock datastore, so we're allocating one.\n", ast_channel_name(chan)); + if (unloading) { + ast_log(LOG_ERROR, "%sLOCK has no datastore and func_lock is unloading, failing.\n", + trylock ? "TRY" : ""); + return -1; + } + lock_store = ast_datastore_alloc(&lock_info, NULL); if (!lock_store) { ast_log(LOG_ERROR, "Unable to allocate new datastore. No locks will be obtained.\n"); @@ -210,6 +218,9 @@ static int get_lock(struct ast_channel *chan, char *lockname, int trylock) lock_store->data = list; AST_LIST_HEAD_INIT(list); ast_channel_datastore_add(chan, lock_store); + + /* We cannot unload until this channel has released the lock_store */ + ast_module_ref(ast_module_info->self); } else list = lock_store->data; @@ -223,6 +234,9 @@ static int get_lock(struct ast_channel *chan, char *lockname, int trylock) if (!current) { if (unloading) { + ast_log(LOG_ERROR, + "Lock doesn't exist whilst unloading. %sLOCK will fail.\n", + trylock ? "TRY" : ""); /* Don't bother */ AST_LIST_UNLOCK(&locklist); return -1; @@ -249,7 +263,6 @@ static int get_lock(struct ast_channel *chan, char *lockname, int trylock) AST_LIST_UNLOCK(&locklist); return -1; } - current->requesters = 0; AST_LIST_INSERT_TAIL(&locklist, current, entries); } /* Add to requester list */ @@ -268,7 +281,13 @@ static int get_lock(struct ast_channel *chan, char *lockname, int trylock) if (!clframe) { if (unloading) { + ast_log(LOG_ERROR, + "Busy unloading. %sLOCK will fail.\n", + trylock ? "TRY" : ""); /* Don't bother */ + ast_mutex_lock(¤t->mutex); + current->requesters--; + ast_mutex_unlock(¤t->mutex); AST_LIST_UNLOCK(list); return -1; } @@ -277,6 +296,9 @@ static int get_lock(struct ast_channel *chan, char *lockname, int trylock) ast_log(LOG_ERROR, "Unable to allocate channel lock frame. %sLOCK will fail.\n", trylock ? "TRY" : ""); + ast_mutex_lock(¤t->mutex); + current->requesters--; + ast_mutex_unlock(¤t->mutex); AST_LIST_UNLOCK(list); return -1; } @@ -409,6 +431,37 @@ static int trylock_read(struct ast_channel *chan, const char *cmd, char *data, c return 0; } +static char *handle_cli_locks_show(struct ast_cli_entry *e, int cmd, struct ast_cli_args *a) +{ + int c = 0; + struct lock_frame* current; + switch (cmd) { + case CLI_INIT: + e->command = "locks show"; + e->usage = + "Usage: locks show\n" + " List all locks known to func_lock, along with their current status.\n"; + return NULL; + case CLI_GENERATE: + return NULL; + } + + ast_cli(a->fd, "func_lock locks:\n"); + ast_cli(a->fd, "%-40s Requesters Owner\n", "Name"); + AST_LIST_LOCK(&locklist); + AST_LIST_TRAVERSE(&locklist, current, entries) { + ast_mutex_lock(¤t->mutex); + ast_cli(a->fd, "%-40s %-10d %s\n", current->name, current->requesters, + current->owner ? ast_channel_name(current->owner) : "(unlocked)"); + ast_mutex_unlock(¤t->mutex); + c++; + } + AST_LIST_UNLOCK(&locklist); + ast_cli(a->fd, "%d total locks listed.\n", c); + + return 0; +} + static struct ast_custom_function lock_function = { .name = "LOCK", .read = lock_read, @@ -427,6 +480,8 @@ static struct ast_custom_function unlock_function = { .read_max = 2, }; +static struct ast_cli_entry cli_locks_show = AST_CLI_DEFINE(handle_cli_locks_show, "List func_lock locks."); + static int unload_module(void) { struct lock_frame *current; @@ -439,10 +494,19 @@ static int unload_module(void) ast_custom_function_unregister(&lock_function); ast_custom_function_unregister(&trylock_function); + ast_cli_unregister(&cli_locks_show); + AST_LIST_LOCK(&locklist); - AST_LIST_TRAVERSE(&locklist, current, entries) { + while ((current = AST_LIST_REMOVE_HEAD(&locklist, entries))) { + int warned = 0; ast_mutex_lock(¤t->mutex); while (current->owner || current->requesters) { + if (!warned) { + ast_log(LOG_WARNING, "Waiting for %d requesters for %s lock %s.\n", + current->requesters, current->owner ? "locked" : "unlocked", + current->name); + warned = 1; + } /* either the mutex is locked, or other parties are currently in get_lock, * we need to wait for all of those to clear first */ ast_cond_wait(¤t->cond, ¤t->mutex); @@ -470,6 +534,7 @@ static int load_module(void) int res = ast_custom_function_register_escalating(&lock_function, AST_CFE_READ); res |= ast_custom_function_register_escalating(&trylock_function, AST_CFE_READ); res |= ast_custom_function_register_escalating(&unlock_function, AST_CFE_READ); + res |= ast_cli_register(&cli_locks_show); return res; }