<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/2741/">https://reviewboard.asterisk.org/r/2741/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 7th, 2013, 11:29 p.m. UTC, <b>rmudgett</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/2741/diff/3/?file=43825#file43825line1609" style="color: black; font-weight: bold; text-decoration: underline;">trunk/main/bridge.c</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">int ast_bridge_impart(struct ast_bridge *bridge, struct ast_channel *chan, struct ast_channel *swap, struct ast_bridge_features *features, int independent)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1579</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="n">independent</span><span class="p">)</span> <span class="p">{</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1608</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="n">independent</span><span class="p">)</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1580</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="cm">/* Independently imparted channels cannot have a PBX. */</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1581</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="n">ast_assert</span><span class="p">(</span><span class="o">!</span><span class="n">ast_channel_pbx</span><span class="p">(</span><span class="n">chan</span><span class="p">));</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1582</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1583</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="n">res</span> <span class="o">=</span> <span class="n">ast_pthread_create_detached</span><span class="p">(</span><span class="o">&</span><span class="n">bridge_channel</span><span class="o">-></span><span class="kr">thread</span><span class="p">,</span> <span class="nb">NULL</span><span class="p">,</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1609</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">res</span> <span class="o">=</span> <span class="n">ast_pthread_create_detached</span><span class="p">(</span><span class="o">&</span><span class="n">bridge_channel</span><span class="o">-></span><span class="kr">thread</span><span class="p">,</span> <span class="nb">NULL</span><span class="p">,</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1584</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">bridge_channel_ind_thread</span><span class="p">,</span> <span class="n">bridge_channel</span><span class="p">);</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1610</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">bridge_channel_ind_thread</span><span class="p">,</span> <span class="n">bridge_channel</span><span class="p">);</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1585</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="p">}</span> <span class="k">else</span> <span class="p">{</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1611</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="p">}</span> <span class="k">else</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1586</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="cm">/* Imparted channels to be departed should not have a PBX either. */</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1587</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="n">ast_assert</span><span class="p">(</span><span class="o">!</span><span class="n">ast_channel_pbx</span><span class="p">(</span><span class="n">chan</span><span class="p">));</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1588</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1589</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="n">res</span> <span class="o">=</span> <span class="n">ast_pthread_create</span><span class="p">(</span><span class="o">&</span><span class="n">bridge_channel</span><span class="o">-></span><span class="kr">thread</span><span class="p">,</span> <span class="nb">NULL</span><span class="p">,</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1612</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">res</span> <span class="o">=</span> <span class="n">ast_pthread_create</span><span class="p">(</span><span class="o">&</span><span class="n">bridge_channel</span><span class="o">-></span><span class="kr">thread</span><span class="p">,</span> <span class="nb">NULL</span><span class="p">,</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1590</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">bridge_channel_depart_thread</span><span class="p">,</span> <span class="n">bridge_channel</span><span class="p">);</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1613</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span><span class="n">bridge_channel_depart_thread</span><span class="p">,</span> <span class="n">bridge_channel</span><span class="p">);</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1591</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="p">}</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1614</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="p">}</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">In the very unlikely event that the impart thread fails to be created, some other thread could complete the channel push into another bridge and we would never pull it from that bridge.</pre>
</blockquote>
<p>On August 7th, 2013, 11:57 p.m. UTC, <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I'm unsure of what else is supposed to be done here.
If the thread can't be created, it should have a non-zero return value. If that occurred, the channel never got pushed into the bridge. res should be set to a non-zero value, and the cleanup code after this point should clean up the channel.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">So, we looked at this some more.
On further inspection, a bridge move operation can't occur while the thread is being spawned. While a call to ast_bridge_add_channel on a separate thread would cause a bridge_move_locked operation to be attempted, bridge_move_locked will fail as the bridge_channel for the channel being moved is not added to the bridge's list of channels until bridge_channel_internal_push is called. bridge_channel_internal_push is not called until bridge_channel_internal_join is called, which occurs after the thread has successfully spawned.
Whew.
That being said, in retrospect, assigning the ast_bridge pointer to the ast_channel in ast_bridge_impart feels a bit wrong.
(1) The management of ast_bridge on the channel is typically handled in bridge_channel. Here, we set it early in bridge.c but clear it in bridge_channel (for a variety of other reasons). Managing this in two places is cumbersome.
(2) It's better to assign the bridge pointer to the channel only when we know it should get into the bridge. That way, things that check for it using the ast_bridge pointer can be reasonably confident that it is, actually, in the bridge.
(3) It isn't actually necessary. While there is a race condition here that the channel could be masqueraded between the initial call to ast_bridge_impart and the thread being spawned, it can't be moved into another bridge (as we just verified). The masquerade can be checked both in ast_bridge_impart and in bridge_channel_internal_join (while it may not be strictly necessary to check for it in both places, it is nice to do so to fail out as quickly as possible if someone stole the channel)</pre>
<br />
<p>- Matt</p>
<br />
<p>On August 7th, 2013, 9:32 p.m. UTC, Matt Jordan wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers and rmudgett.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated Aug. 7, 2013, 9:32 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This patch fixes three problems found using the Bridge AMI action.
The scenario for reproducing the problem was always the same:
* Originate three Local channels into Echo()
* Execute Bridge on Local-0;2 and Local-1;2
* Immediately execute Bridge on Local-1;2 and Local-2;2
This reproduced the following problems:
(1) When originating a channel, the Newchannel event is emitted quickly; however, the channel will not have a pbx thread assigned to it until after the outbound 'dialing' is complete. Thus, there is a period of time where the outside world "knows" of the channel's existence and can influence it but Asterisk has not yet started the dialplan execution thread. If a Bridge AMI action is taken on the channel, the channel appears to be a Dialed channel with no PBX thread; hence, the channel will be imparted into the Bridge. At the same time, another thread that caused the origination will attempt to start a PBX on it. The end result currently is an assertion failure in the Bridging API.
There's no way to prevent AMI from attempting to Bridge a channel immediately after creation; likewise, holding the channel lock through the entire Dial operation is unwise. Instead of treating the presence of a PBX thread as an error, we simply bail out of the adding the channel to the bridge through ast_bridge_impart. The Bridge action will then fail - but we avoid a situation where the channel is both executing a PBX thread and simultaneously being given a separate thread in the bridging system (which would be a "bad thing"). Since imparting a channel with a PBX *can* occur and is not a programming error, the asserts have been removed.
(2) Adding a channel to a bridge consists of two checks: If the channel is in a bridge already, we move the channel using the Bridging API. If not, we steal the channel from its current location and impart it. To know the difference, we check the ast_bridge pointer on the channel. However, if the channel was previously imparted into a Bridge, the ast_bridge pointer is not set on the channel until the impart channel thread has spun up. During that period of time, the channel is unlocked and will appear for all intents and purposes as not being "in" the Bridging API - even though it is just a matter of time until the Bridge takes it on. This patch avoids this race condition by first checking if the channel being imparted is a Zombie - in which case, we bail - and by setting the ast_bridge pointer immediately upon entering the Bridging API. If later actions fail, the ast_bridge pointer is cleared.
(3) bridge_find_channel does not increase the reference count of the ast_bridge_channel object. The RAII_VAR usage in ast_bridge_add_channel thus created a ticking time bomb in whatever bridge the channel moved into, as the destructor for the ast_bridge_channel object would be called.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The nascent AMI Bridge action test stops crashing constantly.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>trunk/main/bridge.c <span style="color: grey">(396364)</span></li>
<li>trunk/main/bridge_channel.c <span style="color: grey">(396364)</span></li>
<li>trunk/main/features.c <span style="color: grey">(396364)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2741/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>