<p>Jaco Kroon has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/15945">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">func_lock: Prevent module unloading in-use module.<br><br>The the scenario where a channel still has an associated datastore we<br>cannot unload since there is a function pointer to the destroy and fixup<br>functions in play. Thus increase the module ref count whenever we<br>allocate a datastore, and decrease it during destroy.<br><br>In order to tighten the race that still exists in spite of this (below)<br>add some extra failure cases to prevent allocations in these cases.<br><br>Race:<br><br>If module ref is zero, an LOCK or TRYLOCK is invoked (near)<br>simultaneously on a channel that has NOT PREVIOUSLY taken a lock, and if<br>in such a case the datastore is created *prior* to unloading being set<br>to true (first step in module unload) then it's possible that the module<br>will unload with the destructor being called (and segfault) post the<br>module being unloaded. The module will however wait for such locks to<br>release prior to unloading.<br><br>If post that we can recheck the module ref before returning the we can<br>(in theory, I think) eliminate the last of the race. This race is<br>mostly theoretical in nature.<br><br>Change-Id: I21a514a0b56755c578a687f4867eacb8b59e23cf<br>Signed-off-by: Jaco Kroon <jaco@uls.co.za><br>---<br>M funcs/func_lock.c<br>1 file changed, 14 insertions(+), 1 deletion(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/45/15945/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/funcs/func_lock.c b/funcs/func_lock.c</span><br><span>index 0726407..85e7937 100644</span><br><span>--- a/funcs/func_lock.c</span><br><span>+++ b/funcs/func_lock.c</span><br><span>@@ -157,6 +157,8 @@</span><br><span> AST_LIST_UNLOCK(oldlist);</span><br><span> AST_LIST_HEAD_DESTROY(oldlist);</span><br><span> ast_free(oldlist);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_module_unref(ast_module_info->self);</span><br><span> }</span><br><span> </span><br><span> static void lock_fixup(void *data, struct ast_channel *oldchan, struct ast_channel *newchan)</span><br><span>@@ -191,7 +193,12 @@</span><br><span> struct timeval now;</span><br><span> </span><br><span> if (!lock_store) {</span><br><span style="color: hsl(0, 100%, 40%);">- ast_debug(1, "Channel %s has no lock datastore, so we're allocating one.\n", ast_channel_name(chan));</span><br><span style="color: hsl(120, 100%, 40%);">+ if (unloading) {</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_log(LOG_ERROR, "%sLOCK has no datastore and func_lock is unloading, failing.\n",</span><br><span style="color: hsl(120, 100%, 40%);">+ trylock ? "TRY" : "");</span><br><span style="color: hsl(120, 100%, 40%);">+ return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> lock_store = ast_datastore_alloc(&lock_info, NULL);</span><br><span> if (!lock_store) {</span><br><span> ast_log(LOG_ERROR, "Unable to allocate new datastore. No locks will be obtained.\n");</span><br><span>@@ -210,6 +217,9 @@</span><br><span> lock_store->data = list;</span><br><span> AST_LIST_HEAD_INIT(list);</span><br><span> ast_channel_datastore_add(chan, lock_store);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* We cannot unload until this channel has released the lock_store */</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_module_ref(ast_module_info->self);</span><br><span> } else</span><br><span> list = lock_store->data;</span><br><span> </span><br><span>@@ -223,6 +233,9 @@</span><br><span> </span><br><span> if (!current) {</span><br><span> if (unloading) {</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_log(LOG_ERROR,</span><br><span style="color: hsl(120, 100%, 40%);">+ "Lock doesn't exist whilst unloading. %sLOCK will fail.\n",</span><br><span style="color: hsl(120, 100%, 40%);">+ trylock ? "TRY" : "");</span><br><span> /* Don't bother */</span><br><span> AST_LIST_UNLOCK(&locklist);</span><br><span> return -1;</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/15945">change 15945</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/+/15945"/><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: I21a514a0b56755c578a687f4867eacb8b59e23cf </div>
<div style="display:none"> Gerrit-Change-Number: 15945 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Jaco Kroon <jaco@uls.co.za> </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>