<p>Kevin Harwell has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.asterisk.org/5889">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">core_local: local channel data not being properly unref'ed and unlocked<br><br>In an earlier version of Asterisk a local channel [un]lock all functions were<br>added in order to keep a crash from occurring when a channel hung up too early<br>during an attended transfer. Unfortunately, when a transfer failure occurs and<br>depending on the timing, the local channels sometime do not get properly<br>unlocked and deref'ed after being locked and ref'ed. This happens because the<br>underlying local channel structure gets NULLed out before unlocking.<br><br>This patch reworks those [un]lock functions and makes sure the values that get<br>locked and ref'ed later get unlocked and deref'ed.<br><br>ASTERISK-27074 #close<br><br>Change-Id: Ice96653e29bd9d6674ed5f95feb6b448ab148b09<br>---<br>M include/asterisk/core_local.h<br>M main/bridge.c<br>M main/core_local.c<br>3 files changed, 93 insertions(+), 22 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/89/5889/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">diff --git a/include/asterisk/core_local.h b/include/asterisk/core_local.h<br>index 8557072..0047577 100644<br>--- a/include/asterisk/core_local.h<br>+++ b/include/asterisk/core_local.h<br>@@ -45,6 +45,8 @@<br>  * \brief Lock the "chan" and "owner" channels (and return them) on the base<br>  *        private structure as well as the base private structure itself.<br>  *<br>+ * \deprecated - *DO NOT USE* Please use ast_local_lock_all2 instead.<br>+ *<br>  * \note This also adds references to each of the above mentioned elements and<br>  *       also the underlying private local structure.<br>  * \note None of these locks should be held prior to calling this function.<br>@@ -60,8 +62,28 @@<br>                       struct ast_channel **outowner);<br> <br> /*!<br>+ * \brief Add a reference to the local channel's private tech, lock the local channel's<br>+ *        private base, and add references and lock both sides of the local channel.<br>+ *<br>+ * \note None of these locks should be held prior to calling this function.<br>+ * \note To undo this process call ast_local_unlock_all2.<br>+ *<br>+ * \since 13.17.0, 14.6.0<br>+ *<br>+ * \param chan Must be a local channel<br>+ * \param tech_pvt [out] channel's private tech (ref added)<br>+ * \param base_pvt [out] channel's private base (lock added)<br>+ * \param base_chan [out] One side of the local channel (ref and lock added)<br>+ * \param base_owner [out] Other side of the local channel (ref and lock added)<br>+ */<br>+void ast_local_lock_all2(struct ast_channel *chan, void **tech_pvt, void **base_pvt,<br>+   struct ast_channel **base_chan, struct ast_channel **base_owner);<br>+<br>+/*!<br>  * \brief Unlock the "chan" and "owner" channels on the base private structure<br>  *        as well as the base private structure itself.<br>+ *<br>+ * \deprecated - *DO NOT USE* Please use ast_local_unlock_all2 instead.<br>  *<br>  * \note This also removes references to each of the above mentioned elements and<br>  *       also the underlying private local structure.<br>@@ -74,6 +96,24 @@<br> void ast_local_unlock_all(struct ast_channel *chan);<br> <br> /*!<br>+ * \brief Remove a reference to the given local channel's private tech, unlock the given<br>+ *        local channel's private base, and remove references and unlock both sides of<br>+ *        given the local channel.<br>+ *<br>+ * \note This function should be used in conjunction with ast_local_lock_all2.<br>+ *<br>+ * \since 13.17.0, 14.6.0<br>+ *<br>+ * \param chan Must be a local channel<br>+ * \param tech_pvt channel's private tech (ref removed)<br>+ * \param base_pvt channel's private base (lock removed)<br>+ * \param base_chan One side of the local channel (ref and lock removed)<br>+ * \param base_owner Other side of the local channel (ref and lock removed)<br>+ */<br>+void ast_local_unlock_all2(struct ast_channel *chan, void *tech_pvt, void *base_pvt,<br>+       struct ast_channel *base_chan, struct ast_channel *base_owner);<br>+<br>+/*!<br>  * \brief Get the other local channel in the pair.<br>  * \since 12.0.0<br>  *<br>diff --git a/main/bridge.c b/main/bridge.c<br>index 740a88f..09d4209 100644<br>--- a/main/bridge.c<br>+++ b/main/bridge.c<br>@@ -4243,14 +4243,15 @@<br>   BRIDGE_LOCK_ONE_OR_BOTH(bridge1, bridge2);<br> <br>         if (bridge2) {<br>+               void *tech, *base;<br>            struct ast_channel *locals[2];<br> <br>             /* Have to lock everything just in case a hangup comes in early */<br>-           ast_local_lock_all(local_chan, &locals[0], &locals[1]);<br>+              ast_local_lock_all2(local_chan, &tech, &base, &locals[0], &locals[1]);<br>                if (!locals[0] || !locals[1]) {<br>                       ast_log(LOG_ERROR, "Transfer failed probably due to an early hangup - "<br>                             "missing other half of '%s'\n", ast_channel_name(local_chan));<br>-                     ast_local_unlock_all(local_chan);<br>+                    ast_local_unlock_all2(local_chan, tech, base, locals[0], locals[1]);<br>                  ao2_cleanup(local_chan);<br>                      return AST_BRIDGE_TRANSFER_FAIL;<br>              }<br>@@ -4261,7 +4262,7 @@<br>              }<br> <br>          ast_attended_transfer_message_add_link(transfer_msg, locals);<br>-                ast_local_unlock_all(local_chan);<br>+            ast_local_unlock_all2(local_chan, tech, base, locals[0], locals[1]);<br>  } else {<br>              ast_attended_transfer_message_add_app(transfer_msg, app, local_chan);<br>         }<br>diff --git a/main/core_local.c b/main/core_local.c<br>index 6644aaf..26f9311 100644<br>--- a/main/core_local.c<br>+++ b/main/core_local.c<br>@@ -235,18 +235,60 @@<br>         char exten[AST_MAX_EXTENSION];<br> };<br> <br>-void ast_local_lock_all(struct ast_channel *chan, struct ast_channel **outchan,<br>-                     struct ast_channel **outowner)<br>+void ast_local_lock_all2(struct ast_channel *chan, void **tech_pvt, void **base_pvt,<br>+                struct ast_channel **base_chan, struct ast_channel **base_owner)<br> {<br>  struct local_pvt *p = ast_channel_tech_pvt(chan);<br> <br>- *outchan = NULL;<br>-     *outowner = NULL;<br>+    *tech_pvt = NULL;<br>+    *base_pvt = NULL;<br>+    *base_chan = NULL;<br>+   *base_owner = NULL;<br> <br>        if (p) {<br>-             ao2_ref(p, 1);<br>-               ast_unreal_lock_all(&p->base, outchan, outowner);<br>+             *tech_pvt = ao2_bump(p);<br>+             *base_pvt = &p->base;<br>+         ast_unreal_lock_all(*base_pvt, base_chan, base_owner);<br>        }<br>+<br>+ ast_debug(5, "Local lock all - tech private = %p, base private = %p, base chan = %p,"<br>+              " base owner = %p\n", *tech_pvt, *base_pvt, *base_chan, *base_owner);<br>+}<br>+<br>+void ast_local_lock_all(struct ast_channel *chan, struct ast_channel **outchan,<br>+                     struct ast_channel **outowner)<br>+{<br>+   void *tech_pvt, *base_pvt;<br>+   ast_local_lock_all2(chan, &tech_pvt, &base_pvt, outchan, outowner);<br>+}<br>+<br>+void ast_local_unlock_all2(struct ast_channel *chan, void *tech_pvt, void *base_pvt,<br>+            struct ast_channel *base_chan, struct ast_channel *base_owner)<br>+{<br>+   struct local_pvt *local = ast_channel_tech_pvt(chan);<br>+        struct ast_unreal_pvt *unreal = local ? &local->base : NULL;<br>+  struct ast_channel *unreal_chan = unreal ? unreal->chan : NULL;<br>+   struct ast_channel *unreal_owner = unreal ? unreal->owner : NULL;<br>+<br>+<br>+   ast_debug(5, "Local unlock all - local private = %p|%p, unreal private = %p|%p, "<br>+          "base chan = %p|%p, base owner = %p|%p\n", local, tech_pvt, unreal,<br>+                base_pvt, unreal_chan, base_chan, unreal_owner, base_owner);<br>+<br>+      if (base_chan) {<br>+             ast_channel_unlock(base_chan);<br>+               ast_channel_unref(base_chan);<br>+        }<br>+<br>+ if (base_owner) {<br>+            ast_channel_unlock(base_owner);<br>+              ast_channel_unref(base_owner);<br>+       }<br>+<br>+ if (base_pvt) {<br>+              ao2_unlock(base_pvt);<br>+        }<br>+    ao2_cleanup(tech_pvt);<br> }<br> <br> void ast_local_unlock_all(struct ast_channel *chan)<br>@@ -259,19 +301,7 @@<br>     }<br> <br>  base = &p->base;<br>-<br>-   if (base->owner) {<br>-                ast_channel_unlock(base->owner);<br>-          ast_channel_unref(base->owner);<br>-   }<br>-<br>- if (base->chan) {<br>-         ast_channel_unlock(base->chan);<br>-           ast_channel_unref(base->chan);<br>-    }<br>-<br>- ao2_unlock(base);<br>-    ao2_ref(p, -1);<br>+      ast_local_unlock_all2(chan, p, base, base->chan, base->owner);<br> }<br> <br> struct ast_channel *ast_local_get_peer(struct ast_channel *ast)<br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/5889">change 5889</a>. To unsubscribe, 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/5889"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 14 </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>
<div style="display:none"> Gerrit-Change-Id: Ice96653e29bd9d6674ed5f95feb6b448ab148b09 </div>
<div style="display:none"> Gerrit-Change-Number: 5889 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Kevin Harwell <kharwell@digium.com> </div>