Skip to content
Snippets Groups Projects
Commit 19a8383a authored by Jaco Kroon's avatar Jaco Kroon Committed by Friendly Automation
Browse files

func_lock: Prevent module unloading in-use module.


The scenario where a channel still has an associated datastore we
cannot unload since there is a function pointer to the destroy and fixup
functions in play.  Thus increase the module ref count whenever we
allocate a datastore, and decrease it during destroy.

In order to tighten the race that still exists in spite of this (below)
add some extra failure cases to prevent allocations in these cases.

Race:

If module ref is zero, an LOCK or TRYLOCK is invoked (near)
simultaneously on a channel that has NOT PREVIOUSLY taken a lock, and if
in such a case the datastore is created *prior* to unloading being set
to true (first step in module unload) then it's possible that the module
will unload with the destructor being called (and segfault) post the
module being unloaded.  The module will however wait for such locks to
release prior to unloading.

If post that we can recheck the module ref before returning the we can
(in theory, I think) eliminate the last of the race.  This race is
mostly theoretical in nature.

Change-Id: I21a514a0b56755c578a687f4867eacb8b59e23cf
Signed-off-by: default avatarJaco Kroon <jaco@uls.co.za>
parent e8875d5c
No related branches found
No related tags found
3 merge requests!138Merge branch asterisk-20.3.0 into devel properly,!123Merge asterisk '20.3.0' into devel,!118Draft: manager: AOC-S support for AOCMessage
...@@ -157,6 +157,8 @@ static void lock_free(void *data) ...@@ -157,6 +157,8 @@ static void lock_free(void *data)
AST_LIST_UNLOCK(oldlist); AST_LIST_UNLOCK(oldlist);
AST_LIST_HEAD_DESTROY(oldlist); AST_LIST_HEAD_DESTROY(oldlist);
ast_free(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) static void lock_fixup(void *data, struct ast_channel *oldchan, struct ast_channel *newchan)
...@@ -191,7 +193,12 @@ static int get_lock(struct ast_channel *chan, char *lockname, int trylock) ...@@ -191,7 +193,12 @@ static int get_lock(struct ast_channel *chan, char *lockname, int trylock)
struct timeval now; struct timeval now;
if (!lock_store) { 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); lock_store = ast_datastore_alloc(&lock_info, NULL);
if (!lock_store) { if (!lock_store) {
ast_log(LOG_ERROR, "Unable to allocate new datastore. No locks will be obtained.\n"); ast_log(LOG_ERROR, "Unable to allocate new datastore. No locks will be obtained.\n");
...@@ -210,6 +217,9 @@ static int get_lock(struct ast_channel *chan, char *lockname, int trylock) ...@@ -210,6 +217,9 @@ static int get_lock(struct ast_channel *chan, char *lockname, int trylock)
lock_store->data = list; lock_store->data = list;
AST_LIST_HEAD_INIT(list); AST_LIST_HEAD_INIT(list);
ast_channel_datastore_add(chan, lock_store); 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 } else
list = lock_store->data; list = lock_store->data;
...@@ -223,6 +233,9 @@ static int get_lock(struct ast_channel *chan, char *lockname, int trylock) ...@@ -223,6 +233,9 @@ static int get_lock(struct ast_channel *chan, char *lockname, int trylock)
if (!current) { if (!current) {
if (unloading) { if (unloading) {
ast_log(LOG_ERROR,
"Lock doesn't exist whilst unloading. %sLOCK will fail.\n",
trylock ? "TRY" : "");
/* Don't bother */ /* Don't bother */
AST_LIST_UNLOCK(&locklist); AST_LIST_UNLOCK(&locklist);
return -1; return -1;
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment