path: root/net-misc/asterisk/files/asterisk-13.38.2-r3-func_lock-fix-races.patch

From b35211fd58afcf430a0d95a243dc7a086d72b2b8 Mon Sep 17 00:00:00 2001
From: Jaco Kroon <jaco@uls.co.za>
Date: Fri, 21 May 2021 20:11:59 +0200
Subject: [PATCH] Replacement patch for v13.

Change-Id: I30236d7d7229f317c681fb7c6d7944d6108acd08
---
 funcs/func_lock.c | 234 +++++++++++++++++++++++++---------------------
 1 file changed, 126 insertions(+), 108 deletions(-)

diff --git a/funcs/func_lock.c b/funcs/func_lock.c
index a006a574eb..c472504f52 100644
--- a/funcs/func_lock.c
+++ b/funcs/func_lock.c
@@ -44,6 +44,7 @@ ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
 #include "asterisk/linkedlists.h"
 #include "asterisk/astobj2.h"
 #include "asterisk/utils.h"
+#include "asterisk/cli.h"
 
 /*** DOCUMENTATION
 	<function name="LOCK" language="en_US">
@@ -112,7 +113,6 @@ static AST_LIST_HEAD_STATIC(locklist, lock_frame);
 static void lock_free(void *data);
 static void lock_fixup(void *data, struct ast_channel *oldchan, struct ast_channel *newchan);
 static int unloading = 0;
-static pthread_t broker_tid = AST_PTHREADT_NULL;
 
 static const struct ast_datastore_info lock_info = {
 	.type = "MUTEX",
@@ -126,8 +126,8 @@ struct lock_frame {
 	ast_cond_t cond;
 	/*! count is needed so if a recursive mutex exits early, we know how many times to unlock it. */
 	unsigned int count;
-	/*! Container of requesters for the named lock */
-	struct ao2_container *requesters;
+	/*! Count of waiting of requesters for the named lock */
+	unsigned int requesters;
 	/*! who owns us */
 	struct ast_channel *owner;
 	/*! name of the lock */
@@ -149,14 +149,19 @@ static void lock_free(void *data)
 	while ((clframe = AST_LIST_REMOVE_HEAD(oldlist, list))) {
 		/* Only unlock if we own the lock */
 		if (clframe->channel == clframe->lock_frame->owner) {
+			ast_mutex_lock(&clframe->lock_frame->mutex);
 			clframe->lock_frame->count = 0;
 			clframe->lock_frame->owner = NULL;
+			ast_cond_signal(&clframe->lock_frame->cond);
+			ast_mutex_unlock(&clframe->lock_frame->mutex);
 		}
 		ast_free(clframe);
 	}
 	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)
@@ -175,54 +180,11 @@ static void lock_fixup(void *data, struct ast_channel *oldchan, struct ast_chann
 		if (clframe->lock_frame->owner == oldchan) {
 			clframe->lock_frame->owner = newchan;
 		}
-		/* We don't move requesters, because the thread stack is different */
 		clframe->channel = newchan;
 	}
 	AST_LIST_UNLOCK(list);
 }
 
-static void *lock_broker(void *unused)
-{
-	struct lock_frame *frame;
-	struct timespec forever = { 1000000, 0 };
-	for (;;) {
-		int found_requester = 0;
-
-		/* Test for cancel outside of the lock */
-		pthread_testcancel();
-		AST_LIST_LOCK(&locklist);
-
-		AST_LIST_TRAVERSE(&locklist, frame, entries) {
-			if (ao2_container_count(frame->requesters)) {
-				found_requester++;
-				ast_mutex_lock(&frame->mutex);
-				if (!frame->owner) {
-					ast_cond_signal(&frame->cond);
-				}
-				ast_mutex_unlock(&frame->mutex);
-			}
-		}
-
-		AST_LIST_UNLOCK(&locklist);
-		pthread_testcancel();
-
-		/* If there are no requesters, then wait for a signal */
-		if (!found_requester) {
-			nanosleep(&forever, NULL);
-		} else {
-			sched_yield();
-		}
-	}
-	/* Not reached */
-	return NULL;
-}
-
-static int ast_channel_cmp_cb(void *obj, void *arg, int flags)
-{
-	struct ast_channel *chan = obj, *cmp_args = arg;
-	return strcasecmp(ast_channel_name(chan), ast_channel_name(cmp_args)) ? 0 : CMP_MATCH;
-}
-
 static int get_lock(struct ast_channel *chan, char *lockname, int trylock)
 {
 	struct ast_datastore *lock_store = ast_channel_datastore_find(chan, &lock_info, NULL);
@@ -234,7 +196,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");
@@ -253,6 +220,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;
 
@@ -266,6 +236,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;
@@ -292,17 +265,12 @@ static int get_lock(struct ast_channel *chan, char *lockname, int trylock)
 			AST_LIST_UNLOCK(&locklist);
 			return -1;
 		}
-		current->requesters = ao2_container_alloc_list(AO2_ALLOC_OPT_LOCK_MUTEX, 0,
-			NULL, ast_channel_cmp_cb);
-		if (!current->requesters) {
-			ast_mutex_destroy(&current->mutex);
-			ast_cond_destroy(&current->cond);
-			ast_free(current);
-			AST_LIST_UNLOCK(&locklist);
-			return -1;
-		}
 		AST_LIST_INSERT_TAIL(&locklist, current, entries);
 	}
+	/* Add to requester list */
+	ast_mutex_lock(&current->mutex);
+	current->requesters++;
+	ast_mutex_unlock(&current->mutex);
 	AST_LIST_UNLOCK(&locklist);
 
 	/* Found lock or created one - now find or create the corresponding link in the channel */
@@ -315,7 +283,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(&current->mutex);
+			current->requesters--;
+			ast_mutex_unlock(&current->mutex);
 			AST_LIST_UNLOCK(list);
 			return -1;
 		}
@@ -324,6 +298,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(&current->mutex);
+			current->requesters--;
+			ast_mutex_unlock(&current->mutex);
 			AST_LIST_UNLOCK(list);
 			return -1;
 		}
@@ -339,44 +316,44 @@ static int get_lock(struct ast_channel *chan, char *lockname, int trylock)
 	 * the same amount, before we'll release this one.
 	 */
 	if (current->owner == chan) {
+		/* We're not a requester, we already have it */
+		ast_mutex_lock(&current->mutex);
+		current->requesters--;
+		ast_mutex_unlock(&current->mutex);
 		current->count++;
 		return 0;
 	}
 
-	/* Okay, we have both frames, so now we need to try to lock.
-	 *
-	 * Locking order: always lock locklist first.  We need the
-	 * locklist lock because the broker thread counts whether
-	 * there are requesters with the locklist lock held, and we
-	 * need to hold it, so that when we send our signal, below,
-	 * to wake up the broker thread, it definitely will see that
-	 * a requester exists at that point in time.  Otherwise, we
-	 * could add to the requesters after it has already seen that
-	 * that lock is unoccupied and wait forever for another signal.
-	 */
-	AST_LIST_LOCK(&locklist);
-	ast_mutex_lock(&current->mutex);
-	/* Add to requester list */
-	ao2_link(current->requesters, chan);
-	pthread_kill(broker_tid, SIGURG);
-	AST_LIST_UNLOCK(&locklist);
-
 	/* Wait up to three seconds from now for LOCK. */
 	now = ast_tvnow();
 	timeout.tv_sec = now.tv_sec + 3;
 	timeout.tv_nsec = now.tv_usec * 1000;
 
-	if (!current->owner
-		|| (!trylock
-			&& !(res = ast_cond_timedwait(&current->cond, &current->mutex, &timeout)))) {
-		res = 0;
+	ast_mutex_lock(&current->mutex);
+
+	res = 0;
+	while (!trylock && !res && current->owner) {
+		res = ast_cond_timedwait(&current->cond, &current->mutex, &timeout);
+	}
+	if (current->owner) {
+		ast_log(LOG_ERROR, "%sLOCK failed to obtain lock %s.\n", trylock ? "TRY" : "",
+				lockname);
+		/* timeout;
+		 * trylock; or
+		 * cond_timedwait failed.
+		 *
+		 * either way, we fail to obtain the lock.
+		 */
+		res = -1;
+	} else {
 		current->owner = chan;
 		current->count++;
-	} else {
-		res = -1;
+		res = 0;
 	}
 	/* Remove from requester list */
-	ao2_unlink(current->requesters, chan);
+	current->requesters--;
+	if (res && unloading)
+		ast_cond_signal(&current->cond);
 	ast_mutex_unlock(&current->mutex);
 
 	return res;
@@ -400,7 +377,7 @@ static int unlock_read(struct ast_channel *chan, const char *cmd, char *data, ch
 	}
 
 	if (!(list = lock_store->data)) {
-		ast_debug(1, "This should NEVER happen\n");
+		ast_log(LOG_ERROR, "Datastore's data member is NULL ... this should be impossible.");
 		ast_copy_string(buf, "0", len);
 		return 0;
 	}
@@ -419,12 +396,17 @@ static int unlock_read(struct ast_channel *chan, const char *cmd, char *data, ch
 
 	if (!clframe) {
 		/* We didn't have this lock in the first place */
+		ast_log(LOG_WARNING, "Attempting to UNLOCK(%s) - a lock this channel never owned.\n",
+				data);
 		ast_copy_string(buf, "0", len);
 		return 0;
 	}
 
 	if (--clframe->lock_frame->count == 0) {
+		ast_mutex_lock(&clframe->lock_frame->mutex);
 		clframe->lock_frame->owner = NULL;
+		ast_cond_signal(&clframe->lock_frame->cond);
+		ast_mutex_unlock(&clframe->lock_frame->mutex);
 	}
 
 	ast_copy_string(buf, "1", len);
@@ -455,6 +437,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(&current->mutex);
+		ast_cli(a->fd, "%-40s %-10d %s\n", current->name, current->requesters,
+				current->owner ? ast_channel_name(current->owner) : "(unlocked)");
+		ast_mutex_unlock(&current->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,
@@ -473,6 +486,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;
@@ -480,34 +495,43 @@ static int unload_module(void)
 	/* Module flag */
 	unloading = 1;
 
+	/* Make it impossible for new requesters to be added
+	 * NOTE:  channels could already be in get_lock() */
+	ast_custom_function_unregister(&lock_function);
+	ast_custom_function_unregister(&trylock_function);
+
+	ast_cli_unregister(&cli_locks_show);
+
 	AST_LIST_LOCK(&locklist);
 	while ((current = AST_LIST_REMOVE_HEAD(&locklist, entries))) {
-		/* If any locks are currently in use, then we cannot unload this module */
-		if (current->owner || ao2_container_count(current->requesters)) {
-			/* Put it back */
-			AST_LIST_INSERT_HEAD(&locklist, current, entries);
-			AST_LIST_UNLOCK(&locklist);
-			unloading = 0;
-			return -1;
+		int warned = 0;
+		ast_mutex_lock(&current->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(&current->cond, &current->mutex);
 		}
+		ast_mutex_unlock(&current->mutex);
+		/* At this point we know:
+		 * 1. the lock has been released,
+		 * 2. there are no requesters (nor should any be able to sneak in).
+		 */
 		ast_mutex_destroy(&current->mutex);
-		ao2_ref(current->requesters, -1);
+		ast_cond_destroy(&current->cond);
 		ast_free(current);
 	}
+	AST_LIST_UNLOCK(&locklist);
+	AST_LIST_HEAD_DESTROY(&locklist);
 
-	/* No locks left, unregister functions */
-	ast_custom_function_unregister(&lock_function);
-	ast_custom_function_unregister(&trylock_function);
+	/* At this point we can safely stop access to UNLOCK */
 	ast_custom_function_unregister(&unlock_function);
 
-	if (broker_tid != AST_PTHREADT_NULL) {
-		pthread_cancel(broker_tid);
-		pthread_kill(broker_tid, SIGURG);
-		pthread_join(broker_tid, NULL);
-	}
-
-	AST_LIST_UNLOCK(&locklist);
-
 	return 0;
 }
 
@@ -516,13 +540,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);
-
-	if (ast_pthread_create_background(&broker_tid, NULL, lock_broker, NULL)) {
-		ast_log(LOG_ERROR, "Failed to start lock broker thread. Unloading func_lock module.\n");
-		broker_tid = AST_PTHREADT_NULL;
-		unload_module();
-		return AST_MODULE_LOAD_DECLINE;
-	}
+	res |= ast_cli_register(&cli_locks_show);
 
 	return res;
 }
-- 
2.26.3