[Asterisk-code-review] pbx_ael: Fix possible crash and serious lockup issue regarding 'ael ... (asterisk[master])

[Mark Murawski]

Mark Murawski has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/16348 )


Change subject: pbx_ael:  Fix possible crash and serious lockup issue regarding 'ael reload'
......................................................................

pbx_ael:  Fix possible crash and serious lockup issue regarding 'ael reload'

Currently pbx_ael does not check if a reload is currently pending
before proceeding with a reload. This can cause multiple threads to
operate at the same time on what should be mutex protected data.  This
change adds a reload mutex and prevents multiple reloads from occuring
simultaneously.

ASTERISK-29609 #close

Change-Id: I5ed392ad226f6e4e7696ad742076d3e45c57af35
---
M pbx/pbx_ael.c
1 file changed, 87 insertions(+), 3 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/48/16348/1

diff --git a/pbx/pbx_ael.c b/pbx/pbx_ael.c
index d55f2d4..608d30c 100644
--- a/pbx/pbx_ael.c
+++ b/pbx/pbx_ael.c
@@ -74,8 +74,21 @@
 #define DEBUG_MACROS (1 << 2)
 #define DEBUG_CONTEXTS (1 << 3)
 
+enum PBX_AEL_LOAD_RESULT {
+  PBX_AEL_LOAD_RESULT_SUCCESS = 0,
+  PBX_AEL_LOAD_RESULT_FAILED,
+  PBX_AEL_LOAD_RESULT_RELOAD_PENDING,
+};
+
+/*! Only allow one reload/unload action to take place at a given time */
+AST_MUTEX_DEFINE_STATIC(ael_load_lock);
+
 static char *config = "extensions.ael";
 static char *registrar = "pbx_ael";
+
+static int pbx_load_module_locked(void);
+static int pbx_load_module_wrapper_locked(void);
+static char *pbx_load_module_from_cli_locked(struct ast_cli_args *a);
 static int pbx_load_module(void);
 
 #ifndef AAL_ARGCHECK
@@ -143,6 +156,72 @@
 
 /* if all the below are static, who cares if they are present? */
 
+/*!
+  \brief So that we can pass this result straight back to callers of load_module
+  \internal
+
+  \retval AST_MODULE_LOAD_SUCCESS Reload worked as intended
+  \retval AST_MODULE_LOAD_DECLINE Reload could not proceed.  Either because of a syntax or other error in AEL or there's already a load in progress
+*/
+static int pbx_load_module_locked(void)
+{
+	int ret;
+
+	ret = pbx_load_module_wrapper_locked();
+
+	return (ret == PBX_AEL_LOAD_RESULT_SUCCESS) ? AST_MODULE_LOAD_SUCCESS : AST_MODULE_LOAD_DECLINE;
+}
+
+/*!
+  \brief Let function callers know exactly why the load failed
+  \internal
+
+  \retval PBX_AEL_LOAD_RESULT_SUCCESS        Reload worked as intended
+  \retval PBX_AEL_LOAD_RESULT_FAILED         Failed (typically due to syntax or other error in AEL files)
+  \retval PBX_AEL_LOAD_RESULT_RELOAD_PENDING Reload already in progress
+*/
+static int pbx_load_module_wrapper_locked(void)
+{
+	int ret;
+
+	if (ast_mutex_trylock(&ael_load_lock)) {
+		return PBX_AEL_LOAD_RESULT_RELOAD_PENDING;
+	};
+
+	ret = pbx_load_module();
+	ast_mutex_unlock(&ael_load_lock);
+
+	if (ret == AST_MODULE_LOAD_SUCCESS) {
+		return PBX_AEL_LOAD_RESULT_SUCCESS;
+	}
+
+	return PBX_AEL_LOAD_RESULT_FAILED;
+}
+
+/*!
+  \brief Give feedback to the original cli function caller about why the load failed
+  \internal
+
+  \retval CLI_SUCCESS  Reload worked as intended
+  \retval CLI_FAILURE  Reload could not proceed.  Either because of a syntax or other error in AEL or there's already a load in progress
+*/
+static char *pbx_load_module_from_cli_locked(struct ast_cli_args *a)
+{
+	int ret;
+
+	ret = pbx_load_module_wrapper_locked();
+
+	if (ret == PBX_AEL_LOAD_RESULT_RELOAD_PENDING) {
+		ast_cli(a->fd, "A module reload request is already in progress; please be patient\n");
+		return CLI_FAILURE;
+	}
+
+	return (ret == PBX_AEL_LOAD_RESULT_SUCCESS) ? CLI_SUCCESS : CLI_FAILURE;
+}
+
+/*
+  Don't call this directly, always use pbx_load_module_locked
+*/
 static int pbx_load_module(void)
 {
 	int errs=0, sem_err=0, sem_warn=0, sem_note=0;
@@ -245,7 +324,7 @@
 	if (a->argc != 2)
 		return CLI_SHOWUSAGE;
 
-	return (pbx_load_module() ? CLI_FAILURE : CLI_SUCCESS);
+	return (pbx_load_module_from_cli_locked(a) ? CLI_FAILURE : CLI_SUCCESS);
 }
 
 static struct ast_cli_entry cli_ael[] = {
@@ -255,11 +334,16 @@
 
 static int unload_module(void)
 {
+	ast_mutex_lock(&ael_load_lock);
 	ast_context_destroy(NULL, registrar);
 	ast_cli_unregister_multiple(cli_ael, ARRAY_LEN(cli_ael));
+
 #ifndef STANDALONE
 	ast_unregister_application(aelsub);
 #endif
+
+	ast_mutex_unlock(&ael_load_lock);
+
 	return 0;
 }
 
@@ -269,12 +353,12 @@
 #ifndef STANDALONE
 	ast_register_application_xml(aelsub, aelsub_exec);
 #endif
-	return (pbx_load_module());
+	return (pbx_load_module_locked());
 }
 
 static int reload(void)
 {
-	return pbx_load_module();
+	return pbx_load_module_locked();
 }
 
 #ifdef STANDALONE

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/16348
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I5ed392ad226f6e4e7696ad742076d3e45c57af35
Gerrit-Change-Number: 16348
Gerrit-PatchSet: 1
Gerrit-Owner: Mark Murawski <markm at intellasoft.net>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210824/aba35a80/attachment-0001.html>