<p>Jaco Kroon has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/15258">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">func_lock: fix multiple-channel-grant problems.<br><br>Under contention it becomes possible that multiple channels will be told<br>they successfully obtained the lock, which is a bug.  Please refer<br><br>ASTERISK-29217<br><br>This introduces a couple of changes.<br><br>1.  Replaces requesters ao2 container with simple counter (we don't<br>    really care who is waiting for the lock, only how many).  This is<br>    updated undex ->mutex to prevent memory access races.<br>2.  Correct semantics for ast_cond_timedwait() as described in<br>    pthread_cond_broadcast(3P) is used (multiple threads can be released<br>    on a single _signal()).<br>3.  Module unload races are taken care of and memory properly cleaned<br>    up.<br><br>Change-Id: I6f68b5ec82ff25b2909daf6e4d19ca864a463e29<br>Signed-off-by: Jaco Kroon <jaco@uls.co.za><br>---<br>M funcs/func_lock.c<br>1 file changed, 58 insertions(+), 109 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/58/15258/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 a006a57..98b2933 100644</span><br><span>--- a/funcs/func_lock.c</span><br><span>+++ b/funcs/func_lock.c</span><br><span>@@ -112,7 +112,6 @@</span><br><span> static void lock_free(void *data);</span><br><span> static void lock_fixup(void *data, struct ast_channel *oldchan, struct ast_channel *newchan);</span><br><span> static int unloading = 0;</span><br><span style="color: hsl(0, 100%, 40%);">-static pthread_t broker_tid = AST_PTHREADT_NULL;</span><br><span> </span><br><span> static const struct ast_datastore_info lock_info = {</span><br><span>      .type = "MUTEX",</span><br><span>@@ -126,8 +125,8 @@</span><br><span>     ast_cond_t cond;</span><br><span>     /*! count is needed so if a recursive mutex exits early, we know how many times to unlock it. */</span><br><span>     unsigned int count;</span><br><span style="color: hsl(0, 100%, 40%);">-     /*! Container of requesters for the named lock */</span><br><span style="color: hsl(0, 100%, 40%);">-       struct ao2_container *requesters;</span><br><span style="color: hsl(120, 100%, 40%);">+     /*! Count of waiting of requesters for the named lock */</span><br><span style="color: hsl(120, 100%, 40%);">+      unsigned int requesters;</span><br><span>     /*! who owns us */</span><br><span>   struct ast_channel *owner;</span><br><span>   /*! name of the lock */</span><br><span>@@ -149,8 +148,11 @@</span><br><span>       while ((clframe = AST_LIST_REMOVE_HEAD(oldlist, list))) {</span><br><span>            /* Only unlock if we own the lock */</span><br><span>                 if (clframe->channel == clframe->lock_frame->owner) {</span><br><span style="color: hsl(120, 100%, 40%);">+                        ast_mutex_lock(&clframe->lock_frame->mutex);</span><br><span>                       clframe->lock_frame->count = 0;</span><br><span>                        clframe->lock_frame->owner = NULL;</span><br><span style="color: hsl(120, 100%, 40%);">+                      ast_cond_signal(&clframe->lock_frame->cond);</span><br><span style="color: hsl(120, 100%, 40%);">+                        ast_mutex_unlock(&clframe->lock_frame->mutex);</span><br><span>             }</span><br><span>            ast_free(clframe);</span><br><span>   }</span><br><span>@@ -175,54 +177,11 @@</span><br><span>            if (clframe->lock_frame->owner == oldchan) {</span><br><span>                   clframe->lock_frame->owner = newchan;</span><br><span>          }</span><br><span style="color: hsl(0, 100%, 40%);">-               /* We don't move requesters, because the thread stack is different */</span><br><span>            clframe->channel = newchan;</span><br><span>       }</span><br><span>    AST_LIST_UNLOCK(list);</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-static void *lock_broker(void *unused)</span><br><span style="color: hsl(0, 100%, 40%);">-{</span><br><span style="color: hsl(0, 100%, 40%);">-  struct lock_frame *frame;</span><br><span style="color: hsl(0, 100%, 40%);">-       struct timespec forever = { 1000000, 0 };</span><br><span style="color: hsl(0, 100%, 40%);">-       for (;;) {</span><br><span style="color: hsl(0, 100%, 40%);">-              int found_requester = 0;</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">-                /* Test for cancel outside of the lock */</span><br><span style="color: hsl(0, 100%, 40%);">-               pthread_testcancel();</span><br><span style="color: hsl(0, 100%, 40%);">-           AST_LIST_LOCK(&locklist);</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">-           AST_LIST_TRAVERSE(&locklist, frame, entries) {</span><br><span style="color: hsl(0, 100%, 40%);">-                      if (ao2_container_count(frame->requesters)) {</span><br><span style="color: hsl(0, 100%, 40%);">-                                found_requester++;</span><br><span style="color: hsl(0, 100%, 40%);">-                              ast_mutex_lock(&frame->mutex);</span><br><span style="color: hsl(0, 100%, 40%);">-                           if (!frame->owner) {</span><br><span style="color: hsl(0, 100%, 40%);">-                                 ast_cond_signal(&frame->cond);</span><br><span style="color: hsl(0, 100%, 40%);">-                           }</span><br><span style="color: hsl(0, 100%, 40%);">-                               ast_mutex_unlock(&frame->mutex);</span><br><span style="color: hsl(0, 100%, 40%);">-                 }</span><br><span style="color: hsl(0, 100%, 40%);">-               }</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">-               AST_LIST_UNLOCK(&locklist);</span><br><span style="color: hsl(0, 100%, 40%);">-         pthread_testcancel();</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">-           /* If there are no requesters, then wait for a signal */</span><br><span style="color: hsl(0, 100%, 40%);">-                if (!found_requester) {</span><br><span style="color: hsl(0, 100%, 40%);">-                 nanosleep(&forever, NULL);</span><br><span style="color: hsl(0, 100%, 40%);">-          } else {</span><br><span style="color: hsl(0, 100%, 40%);">-                        sched_yield();</span><br><span style="color: hsl(0, 100%, 40%);">-          }</span><br><span style="color: hsl(0, 100%, 40%);">-       }</span><br><span style="color: hsl(0, 100%, 40%);">-       /* Not reached */</span><br><span style="color: hsl(0, 100%, 40%);">-       return NULL;</span><br><span style="color: hsl(0, 100%, 40%);">-}</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">-static int ast_channel_cmp_cb(void *obj, void *arg, int flags)</span><br><span style="color: hsl(0, 100%, 40%);">-{</span><br><span style="color: hsl(0, 100%, 40%);">-    struct ast_channel *chan = obj, *cmp_args = arg;</span><br><span style="color: hsl(0, 100%, 40%);">-        return strcasecmp(ast_channel_name(chan), ast_channel_name(cmp_args)) ? 0 : CMP_MATCH;</span><br><span style="color: hsl(0, 100%, 40%);">-}</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> static int get_lock(struct ast_channel *chan, char *lockname, int trylock)</span><br><span> {</span><br><span>   struct ast_datastore *lock_store = ast_channel_datastore_find(chan, &lock_info, NULL);</span><br><span>@@ -292,17 +251,13 @@</span><br><span>                   AST_LIST_UNLOCK(&locklist);</span><br><span>                      return -1;</span><br><span>           }</span><br><span style="color: hsl(0, 100%, 40%);">-               current->requesters = ao2_container_alloc_list(AO2_ALLOC_OPT_LOCK_MUTEX, 0,</span><br><span style="color: hsl(0, 100%, 40%);">-                  NULL, ast_channel_cmp_cb);</span><br><span style="color: hsl(0, 100%, 40%);">-              if (!current->requesters) {</span><br><span style="color: hsl(0, 100%, 40%);">-                  ast_mutex_destroy(&current->mutex);</span><br><span style="color: hsl(0, 100%, 40%);">-                      ast_cond_destroy(&current->cond);</span><br><span style="color: hsl(0, 100%, 40%);">-                        ast_free(current);</span><br><span style="color: hsl(0, 100%, 40%);">-                      AST_LIST_UNLOCK(&locklist);</span><br><span style="color: hsl(0, 100%, 40%);">-                 return -1;</span><br><span style="color: hsl(0, 100%, 40%);">-              }</span><br><span style="color: hsl(120, 100%, 40%);">+             current->requesters = 0;</span><br><span>          AST_LIST_INSERT_TAIL(&locklist, current, entries);</span><br><span>       }</span><br><span style="color: hsl(120, 100%, 40%);">+     /* Add to requester list */</span><br><span style="color: hsl(120, 100%, 40%);">+   ast_mutex_lock(&current->mutex);</span><br><span style="color: hsl(120, 100%, 40%);">+       current->requesters++;</span><br><span style="color: hsl(120, 100%, 40%);">+     ast_mutex_unlock(&current->mutex);</span><br><span>    AST_LIST_UNLOCK(&locklist);</span><br><span> </span><br><span>  /* Found lock or created one - now find or create the corresponding link in the channel */</span><br><span>@@ -339,44 +294,42 @@</span><br><span>    * the same amount, before we'll release this one.</span><br><span>        */</span><br><span>  if (current->owner == chan) {</span><br><span style="color: hsl(120, 100%, 40%);">+              /* We're not a requester, we already have it */</span><br><span style="color: hsl(120, 100%, 40%);">+           ast_mutex_lock(&current->mutex);</span><br><span style="color: hsl(120, 100%, 40%);">+               current->requesters--;</span><br><span style="color: hsl(120, 100%, 40%);">+             ast_mutex_unlock(&current->mutex);</span><br><span>            current->count++;</span><br><span>                 return 0;</span><br><span>    }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-   /* Okay, we have both frames, so now we need to try to lock.</span><br><span style="color: hsl(0, 100%, 40%);">-     *</span><br><span style="color: hsl(0, 100%, 40%);">-       * Locking order: always lock locklist first.  We need the</span><br><span style="color: hsl(0, 100%, 40%);">-       * locklist lock because the broker thread counts whether</span><br><span style="color: hsl(0, 100%, 40%);">-        * there are requesters with the locklist lock held, and we</span><br><span style="color: hsl(0, 100%, 40%);">-      * need to hold it, so that when we send our signal, below,</span><br><span style="color: hsl(0, 100%, 40%);">-      * to wake up the broker thread, it definitely will see that</span><br><span style="color: hsl(0, 100%, 40%);">-     * a requester exists at that point in time.  Otherwise, we</span><br><span style="color: hsl(0, 100%, 40%);">-      * could add to the requesters after it has already seen that</span><br><span style="color: hsl(0, 100%, 40%);">-    * that lock is unoccupied and wait forever for another signal.</span><br><span style="color: hsl(0, 100%, 40%);">-  */</span><br><span style="color: hsl(0, 100%, 40%);">-     AST_LIST_LOCK(&locklist);</span><br><span style="color: hsl(0, 100%, 40%);">-   ast_mutex_lock(&current->mutex);</span><br><span style="color: hsl(0, 100%, 40%);">- /* Add to requester list */</span><br><span style="color: hsl(0, 100%, 40%);">-     ao2_link(current->requesters, chan);</span><br><span style="color: hsl(0, 100%, 40%);">- pthread_kill(broker_tid, SIGURG);</span><br><span style="color: hsl(0, 100%, 40%);">-       AST_LIST_UNLOCK(&locklist);</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span>      /* Wait up to three seconds from now for LOCK. */</span><br><span>    now = ast_tvnow();</span><br><span>   timeout.tv_sec = now.tv_sec + 3;</span><br><span>     timeout.tv_nsec = now.tv_usec * 1000;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-       if (!current->owner</span><br><span style="color: hsl(0, 100%, 40%);">-          || (!trylock</span><br><span style="color: hsl(0, 100%, 40%);">-                    && !(res = ast_cond_timedwait(&current->cond, &current->mutex, &timeout)))) {</span><br><span style="color: hsl(0, 100%, 40%);">-         res = 0;</span><br><span style="color: hsl(120, 100%, 40%);">+      ast_mutex_lock(&current->mutex);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+     res = 0;</span><br><span style="color: hsl(120, 100%, 40%);">+      while (!trylock && !res && current->owner) {</span><br><span style="color: hsl(120, 100%, 40%);">+               res = ast_cond_timedwait(&current->cond, &current->mutex, &timeout);</span><br><span style="color: hsl(120, 100%, 40%);">+        }</span><br><span style="color: hsl(120, 100%, 40%);">+     if (current->owner) {</span><br><span style="color: hsl(120, 100%, 40%);">+              /* timeout;</span><br><span style="color: hsl(120, 100%, 40%);">+            * trylock; or</span><br><span style="color: hsl(120, 100%, 40%);">+                 * cond_timedwait failed.</span><br><span style="color: hsl(120, 100%, 40%);">+              *</span><br><span style="color: hsl(120, 100%, 40%);">+             * either way, we fail to obtain the lock.</span><br><span style="color: hsl(120, 100%, 40%);">+             */</span><br><span style="color: hsl(120, 100%, 40%);">+           res = -1;</span><br><span style="color: hsl(120, 100%, 40%);">+     } else {</span><br><span>             current->owner = chan;</span><br><span>            current->count++;</span><br><span style="color: hsl(0, 100%, 40%);">-    } else {</span><br><span style="color: hsl(0, 100%, 40%);">-                res = -1;</span><br><span style="color: hsl(120, 100%, 40%);">+             res = 0;</span><br><span>     }</span><br><span>    /* Remove from requester list */</span><br><span style="color: hsl(0, 100%, 40%);">-        ao2_unlink(current->requesters, chan);</span><br><span style="color: hsl(120, 100%, 40%);">+     current->requesters--;</span><br><span style="color: hsl(120, 100%, 40%);">+     if (res && unloading)</span><br><span style="color: hsl(120, 100%, 40%);">+         ast_cond_signal(&current->cond);</span><br><span>      ast_mutex_unlock(&current->mutex);</span><br><span> </span><br><span>        return res;</span><br><span>@@ -424,7 +377,10 @@</span><br><span>   }</span><br><span> </span><br><span>        if (--clframe->lock_frame->count == 0) {</span><br><span style="color: hsl(120, 100%, 40%);">+                ast_mutex_lock(&clframe->lock_frame->mutex);</span><br><span>               clframe->lock_frame->owner = NULL;</span><br><span style="color: hsl(120, 100%, 40%);">+              ast_cond_signal(&clframe->lock_frame->cond);</span><br><span style="color: hsl(120, 100%, 40%);">+                ast_mutex_unlock(&clframe->lock_frame->mutex);</span><br><span>     }</span><br><span> </span><br><span>        ast_copy_string(buf, "1", len);</span><br><span>@@ -480,33 +436,33 @@</span><br><span>    /* Module flag */</span><br><span>    unloading = 1;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-      AST_LIST_LOCK(&locklist);</span><br><span style="color: hsl(0, 100%, 40%);">-   while ((current = AST_LIST_REMOVE_HEAD(&locklist, entries))) {</span><br><span style="color: hsl(0, 100%, 40%);">-              /* If any locks are currently in use, then we cannot unload this module */</span><br><span style="color: hsl(0, 100%, 40%);">-              if (current->owner || ao2_container_count(current->requesters)) {</span><br><span style="color: hsl(0, 100%, 40%);">-                 /* Put it back */</span><br><span style="color: hsl(0, 100%, 40%);">-                       AST_LIST_INSERT_HEAD(&locklist, current, entries);</span><br><span style="color: hsl(0, 100%, 40%);">-                  AST_LIST_UNLOCK(&locklist);</span><br><span style="color: hsl(0, 100%, 40%);">-                 unloading = 0;</span><br><span style="color: hsl(0, 100%, 40%);">-                  return -1;</span><br><span style="color: hsl(0, 100%, 40%);">-              }</span><br><span style="color: hsl(0, 100%, 40%);">-               ast_mutex_destroy(&current->mutex);</span><br><span style="color: hsl(0, 100%, 40%);">-              ao2_ref(current->requesters, -1);</span><br><span style="color: hsl(0, 100%, 40%);">-            ast_free(current);</span><br><span style="color: hsl(0, 100%, 40%);">-      }</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">-       /* No locks left, unregister functions */</span><br><span style="color: hsl(120, 100%, 40%);">+     /* Make it impossible for new requesters to be added</span><br><span style="color: hsl(120, 100%, 40%);">+   * NOTE:  channels could already be in get_lock() */</span><br><span>         ast_custom_function_unregister(&lock_function);</span><br><span>  ast_custom_function_unregister(&trylock_function);</span><br><span style="color: hsl(0, 100%, 40%);">-  ast_custom_function_unregister(&unlock_function);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-       if (broker_tid != AST_PTHREADT_NULL) {</span><br><span style="color: hsl(0, 100%, 40%);">-          pthread_cancel(broker_tid);</span><br><span style="color: hsl(0, 100%, 40%);">-             pthread_kill(broker_tid, SIGURG);</span><br><span style="color: hsl(0, 100%, 40%);">-               pthread_join(broker_tid, NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+       AST_LIST_LOCK(&locklist);</span><br><span style="color: hsl(120, 100%, 40%);">+ AST_LIST_TRAVERSE(&locklist, current, entries) {</span><br><span style="color: hsl(120, 100%, 40%);">+          ast_mutex_lock(&current->mutex);</span><br><span style="color: hsl(120, 100%, 40%);">+               while (current->owner || current->requesters) {</span><br><span style="color: hsl(120, 100%, 40%);">+                 /* either the mutex is locked, or other parties are currently in get_lock,</span><br><span style="color: hsl(120, 100%, 40%);">+                     * we need to wait for all of those to clear first */</span><br><span style="color: hsl(120, 100%, 40%);">+                 ast_cond_wait(&current->cond, &current->mutex);</span><br><span style="color: hsl(120, 100%, 40%);">+         }</span><br><span style="color: hsl(120, 100%, 40%);">+             ast_mutex_unlock(&current->mutex);</span><br><span style="color: hsl(120, 100%, 40%);">+             /* At this point we know:</span><br><span style="color: hsl(120, 100%, 40%);">+              * 1. the lock has been released,</span><br><span style="color: hsl(120, 100%, 40%);">+              * 2. there are no requesters (nor should any be able to sneak in).</span><br><span style="color: hsl(120, 100%, 40%);">+            */</span><br><span style="color: hsl(120, 100%, 40%);">+           ast_mutex_destroy(&current->mutex);</span><br><span style="color: hsl(120, 100%, 40%);">+            ast_cond_destroy(&current->cond);</span><br><span style="color: hsl(120, 100%, 40%);">+              ast_free(current);</span><br><span>   }</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span>    AST_LIST_UNLOCK(&locklist);</span><br><span style="color: hsl(120, 100%, 40%);">+       AST_LIST_HEAD_DESTROY(&locklist);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+       /* At this point we can safely stop access to UNLOCK */</span><br><span style="color: hsl(120, 100%, 40%);">+       ast_custom_function_unregister(&unlock_function);</span><br><span> </span><br><span>    return 0;</span><br><span> }</span><br><span>@@ -517,13 +473,6 @@</span><br><span>        res |= ast_custom_function_register_escalating(&trylock_function, AST_CFE_READ);</span><br><span>         res |= ast_custom_function_register_escalating(&unlock_function, AST_CFE_READ);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (ast_pthread_create_background(&broker_tid, NULL, lock_broker, NULL)) {</span><br><span style="color: hsl(0, 100%, 40%);">-          ast_log(LOG_ERROR, "Failed to start lock broker thread. Unloading func_lock module.\n");</span><br><span style="color: hsl(0, 100%, 40%);">-              broker_tid = AST_PTHREADT_NULL;</span><br><span style="color: hsl(0, 100%, 40%);">-         unload_module();</span><br><span style="color: hsl(0, 100%, 40%);">-                return AST_MODULE_LOAD_DECLINE;</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span>    return res;</span><br><span> }</span><br><span> </span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/15258">change 15258</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/+/15258"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-Change-Id: I6f68b5ec82ff25b2909daf6e4d19ca864a463e29 </div>
<div style="display:none"> Gerrit-Change-Number: 15258 </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>