<p>Mark Murawski has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/16348">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">pbx_ael: Fix possible crash and serious lockup issue regarding 'ael reload'<br><br>Currently pbx_ael does not check if a reload is currently pending<br>before proceeding with a reload. This can cause multiple threads to<br>operate at the same time on what should be mutex protected data. This<br>change adds a reload mutex and prevents multiple reloads from occuring<br>simultaneously.<br><br>ASTERISK-29609 #close<br><br>Change-Id: I5ed392ad226f6e4e7696ad742076d3e45c57af35<br>---<br>M pbx/pbx_ael.c<br>1 file changed, 87 insertions(+), 3 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/48/16348/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/pbx/pbx_ael.c b/pbx/pbx_ael.c</span><br><span>index d55f2d4..608d30c 100644</span><br><span>--- a/pbx/pbx_ael.c</span><br><span>+++ b/pbx/pbx_ael.c</span><br><span>@@ -74,8 +74,21 @@</span><br><span> #define DEBUG_MACROS (1 << 2)</span><br><span> #define DEBUG_CONTEXTS (1 << 3)</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+enum PBX_AEL_LOAD_RESULT {</span><br><span style="color: hsl(120, 100%, 40%);">+ PBX_AEL_LOAD_RESULT_SUCCESS = 0,</span><br><span style="color: hsl(120, 100%, 40%);">+ PBX_AEL_LOAD_RESULT_FAILED,</span><br><span style="color: hsl(120, 100%, 40%);">+ PBX_AEL_LOAD_RESULT_RELOAD_PENDING,</span><br><span style="color: hsl(120, 100%, 40%);">+};</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+/*! Only allow one reload/unload action to take place at a given time */</span><br><span style="color: hsl(120, 100%, 40%);">+AST_MUTEX_DEFINE_STATIC(ael_load_lock);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> static char *config = "extensions.ael";</span><br><span> static char *registrar = "pbx_ael";</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+static int pbx_load_module_locked(void);</span><br><span style="color: hsl(120, 100%, 40%);">+static int pbx_load_module_wrapper_locked(void);</span><br><span style="color: hsl(120, 100%, 40%);">+static char *pbx_load_module_from_cli_locked(struct ast_cli_args *a);</span><br><span> static int pbx_load_module(void);</span><br><span> </span><br><span> #ifndef AAL_ARGCHECK</span><br><span>@@ -143,6 +156,72 @@</span><br><span> </span><br><span> /* if all the below are static, who cares if they are present? */</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+/*!</span><br><span style="color: hsl(120, 100%, 40%);">+ \brief So that we can pass this result straight back to callers of load_module</span><br><span style="color: hsl(120, 100%, 40%);">+ \internal</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ \retval AST_MODULE_LOAD_SUCCESS Reload worked as intended</span><br><span style="color: hsl(120, 100%, 40%);">+ \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</span><br><span style="color: hsl(120, 100%, 40%);">+*/</span><br><span style="color: hsl(120, 100%, 40%);">+static int pbx_load_module_locked(void)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ int ret;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ ret = pbx_load_module_wrapper_locked();</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ return (ret == PBX_AEL_LOAD_RESULT_SUCCESS) ? AST_MODULE_LOAD_SUCCESS : AST_MODULE_LOAD_DECLINE;</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+/*!</span><br><span style="color: hsl(120, 100%, 40%);">+ \brief Let function callers know exactly why the load failed</span><br><span style="color: hsl(120, 100%, 40%);">+ \internal</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ \retval PBX_AEL_LOAD_RESULT_SUCCESS Reload worked as intended</span><br><span style="color: hsl(120, 100%, 40%);">+ \retval PBX_AEL_LOAD_RESULT_FAILED Failed (typically due to syntax or other error in AEL files)</span><br><span style="color: hsl(120, 100%, 40%);">+ \retval PBX_AEL_LOAD_RESULT_RELOAD_PENDING Reload already in progress</span><br><span style="color: hsl(120, 100%, 40%);">+*/</span><br><span style="color: hsl(120, 100%, 40%);">+static int pbx_load_module_wrapper_locked(void)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ int ret;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ast_mutex_trylock(&ael_load_lock)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ return PBX_AEL_LOAD_RESULT_RELOAD_PENDING;</span><br><span style="color: hsl(120, 100%, 40%);">+ };</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ ret = pbx_load_module();</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_unlock(&ael_load_lock);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ret == AST_MODULE_LOAD_SUCCESS) {</span><br><span style="color: hsl(120, 100%, 40%);">+ return PBX_AEL_LOAD_RESULT_SUCCESS;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ return PBX_AEL_LOAD_RESULT_FAILED;</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+/*!</span><br><span style="color: hsl(120, 100%, 40%);">+ \brief Give feedback to the original cli function caller about why the load failed</span><br><span style="color: hsl(120, 100%, 40%);">+ \internal</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ \retval CLI_SUCCESS Reload worked as intended</span><br><span style="color: hsl(120, 100%, 40%);">+ \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</span><br><span style="color: hsl(120, 100%, 40%);">+*/</span><br><span style="color: hsl(120, 100%, 40%);">+static char *pbx_load_module_from_cli_locked(struct ast_cli_args *a)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ int ret;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ ret = pbx_load_module_wrapper_locked();</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ret == PBX_AEL_LOAD_RESULT_RELOAD_PENDING) {</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_cli(a->fd, "A module reload request is already in progress; please be patient\n");</span><br><span style="color: hsl(120, 100%, 40%);">+ return CLI_FAILURE;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ return (ret == PBX_AEL_LOAD_RESULT_SUCCESS) ? CLI_SUCCESS : CLI_FAILURE;</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+/*</span><br><span style="color: hsl(120, 100%, 40%);">+ Don't call this directly, always use pbx_load_module_locked</span><br><span style="color: hsl(120, 100%, 40%);">+*/</span><br><span> static int pbx_load_module(void)</span><br><span> {</span><br><span> int errs=0, sem_err=0, sem_warn=0, sem_note=0;</span><br><span>@@ -245,7 +324,7 @@</span><br><span> if (a->argc != 2)</span><br><span> return CLI_SHOWUSAGE;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- return (pbx_load_module() ? CLI_FAILURE : CLI_SUCCESS);</span><br><span style="color: hsl(120, 100%, 40%);">+ return (pbx_load_module_from_cli_locked(a) ? CLI_FAILURE : CLI_SUCCESS);</span><br><span> }</span><br><span> </span><br><span> static struct ast_cli_entry cli_ael[] = {</span><br><span>@@ -255,11 +334,16 @@</span><br><span> </span><br><span> static int unload_module(void)</span><br><span> {</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_lock(&ael_load_lock);</span><br><span> ast_context_destroy(NULL, registrar);</span><br><span> ast_cli_unregister_multiple(cli_ael, ARRAY_LEN(cli_ael));</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> #ifndef STANDALONE</span><br><span> ast_unregister_application(aelsub);</span><br><span> #endif</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_mutex_unlock(&ael_load_lock);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> return 0;</span><br><span> }</span><br><span> </span><br><span>@@ -269,12 +353,12 @@</span><br><span> #ifndef STANDALONE</span><br><span> ast_register_application_xml(aelsub, aelsub_exec);</span><br><span> #endif</span><br><span style="color: hsl(0, 100%, 40%);">- return (pbx_load_module());</span><br><span style="color: hsl(120, 100%, 40%);">+ return (pbx_load_module_locked());</span><br><span> }</span><br><span> </span><br><span> static int reload(void)</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">- return pbx_load_module();</span><br><span style="color: hsl(120, 100%, 40%);">+ return pbx_load_module_locked();</span><br><span> }</span><br><span> </span><br><span> #ifdef STANDALONE</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/16348">change 16348</a>. To unsubscribe, or for help writing mail filters, 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/c/asterisk/+/16348"/><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-Change-Id: I5ed392ad226f6e4e7696ad742076d3e45c57af35 </div>
<div style="display:none"> Gerrit-Change-Number: 16348 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Mark Murawski <markm@intellasoft.net> </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>