<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/2961/">https://reviewboard.asterisk.org/r/2961/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">So, it appears that I applied the incorrect diff to this review, and that is why it seems that some of your comments from before were not addressed.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 19th, 2013, 3:02 a.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/2961/diff/2/?file=48220#file48220line580" style="color: black; font-weight: bold; text-decoration: underline;">/branches/12/channels/chan_pjsip.c</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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; ">static struct ast_channel *chan_pjsip_new(struct ast_sip_session *session, int state, const char *exten, const char *title, const char *linkedid, const char *cid_name)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">580</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ast_channel_lock</span><span class="p">(</span><span class="n">chan</span><span class="p">);</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">580</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ast_channel_stage_snapshot</span><span class="p">(</span><span class="n">chan</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">581</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ast_channel_stage_snapshot</span><span class="p">(</span><span class="n">chan</span><span class="p">);</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">581</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">582</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">582</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="cm">/* If res_pjsip_session is ever updated to create/destroy ast_sip_session_media</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">583</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="cm">/* If res_pjsip_session is ever updated to create/destroy ast_sip_session_media</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">583</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"><span class="tb"> </span> * during a call such as if multiple same-type stream support is introduced,</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">584</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"><span class="tb"> </span> * during a call such as if multiple same-type stream support is introduced,</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">584</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"><span class="tb"> </span> * these will need to be recaptured as well */</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">585</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"><span class="tb"> </span> * these will need to be recaptured as well */</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">585</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">pvt</span><span class="o">-></span><span class="n">media</span><span class="p">[</span><span class="n">SIP_MEDIA_AUDIO</span><span class="p">]</span> <span class="o">=</span> <span class="n">ao2_find</span><span class="p">(</span><span class="n">session</span><span class="o">-></span><span class="n">media</span><span class="p">,</span> <span class="s">"audio"</span><span class="p">,</span> <span class="n">OBJ_KEY</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">586</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">pvt</span><span class="o">-></span><span class="n">media</span><span class="p">[</span><span class="n">SIP_MEDIA_AUDIO</span><span class="p">]</span> <span class="o">=</span> <span class="n">ao2_find</span><span class="p">(</span><span class="n">session</span><span class="o">-></span><span class="n">media</span><span class="p">,</span> <span class="s">"audio"</span><span class="p">,</span> <span class="n">OBJ_KEY</span><span class="p">);</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">586</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">pvt</span><span class="o">-></span><span class="n">media</span><span class="p">[</span><span class="n">SIP_MEDIA_VIDEO</span><span class="p">]</span> <span class="o">=</span> <span class="n">ao2_find</span><span class="p">(</span><span class="n">session</span><span class="o">-></span><span class="n">media</span><span class="p">,</span> <span class="s">"video"</span><span class="p">,</span> <span class="n">OBJ_KEY</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">587</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">pvt</span><span class="o">-></span><span class="n">media</span><span class="p">[</span><span class="n">SIP_MEDIA_VIDEO</span><span class="p">]</span> <span class="o">=</span> <span class="n">ao2_find</span><span class="p">(</span><span class="n">session</span><span class="o">-></span><span class="n">media</span><span class="p">,</span> <span class="s">"video"</span><span class="p">,</span> <span class="n">OBJ_KEY</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;">Is it safe to do these finds with the channel locked?</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;">It indeed is not. Good catch. I'll fix it by moving the ao2_finds to before the channel is locked.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 19th, 2013, 3:02 a.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/2961/diff/2/?file=48249#file48249line3688" style="color: black; font-weight: bold; text-decoration: underline;">/branches/12/res/res_agi.c</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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; ">static void publish_async_exec_end(struct ast_channel *chan, int command_id, const char *command, int result_code, const char *result)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">3678</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ast_channel_publish_blob</span><span class="p">(</span><span class="n">chan</span><span class="p">,</span> <span class="n">agi_exec_start_type</span><span class="p">(),</span> <span class="n">startblob</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">3688</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">ast_channel_publish_blob</span><span class="p">(</span><span class="n">chan</span><span class="p">,</span> <span class="n">agi_exec_start_type</span><span class="p">(),</span> <span class="n">startblob</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;">Need to lock chan around this call.
Check if these other places need locking added:
aoc.c:1874
chan_sip.c:28824
channel.c:3629
channel.c:3646
pbx.c:5793
stasis_channels.c:667</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;">I fixed the res_agi.c call.
In all of the other places you've mentioned, the channel is already locked when control reaches the line you pointed out. In the case of ast_aoc_manager_event() and ast_channel_publish_varset(), they have had their documentation altered to indicate the precondition of having the channel locked.</pre>
<br />
<p>- Mark</p>
<br />
<p>On November 13th, 2013, 6:59 p.m. UTC, Mark Michelson 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.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Mark Michelson.</div>
<p style="color: grey;"><i>Updated Nov. 13, 2013, 6:59 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-22709">ASTERISK-22709</a>
</div>
<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;">John Bigelow discovered that if the newly-created atxfer_threeway_nominal test is run in a loop, eventually a test run will cause the UUT instance of Asterisk to crash. Investigation of the crash showed that the crash was being caused while trying to get the string representation of a channel's translation paths while creating a channel snapshot. The attended transfer manager thread was creating the channel snapshot as part of the attended transfer stasis message while in another thread (in this case, the bridge channel thread) the translation paths were being altered to make the channel compatible with others in the bridge.
It's clear that the issue is that the channel's contents are not lock protected while creating a channel snapshot. Luckily, the channel's lock is held already when updating translation paths. By adding locking, I was able to run the test over 450 times straight with no negative consequences.
When adding locking, there were two strategies to follow:
1) Take advantage of the fact that channel locks are recursive, and just add a single SCOPED_CHANNELLOCK() call to ast_channel_snapshot_create() and call it a day.
2) Make an audit of channel-related calls that end up creating a channel snapshot and require locks be held prior to making those calls.
I like approach 2 more, even though it's harder to pull off. In investigating what paths result in channel snapshots being created, I found that channel locks should be required before calling the following functions:
* ast_channel_snapshot_create()
* ast_channel_blob_create()
* ast_channel_publish_snapshot()
* ast_publish_channel_state()
* ast_channel_publish_blob()
* ast_channel_publish_varset()
* ast_channel_amaflags_set()
* ast_channel_callid_set()
* ast_channel_whentohangup_set()
* ast_channel_callgroup_set()
* ast_channel_pickupgroup_set()
* ast_channel_internal_bridge_set()
* ast_channel_language_set()
* ast_channel_accountcode_set()
* ast_channel_peeraccount_set()
* ast_channel_linkedid_set()
* ast_channel_stage_snapshot()
* ast_setstate()
* ast_aoc_manager_event()
* ast_channel_setwhentohangup_tv()
* ast_channel_setwhentohangup()
* ast_set_variables()
All of these functions' documentation have been updated to indicate a precondition of having the channel locked.
In addition, there are some more complex functions that create channel snapshots but could not easily be made to have the precondition of having the channel locked. These are:
* ast_bridge_blob_create()
* ast_publish_attended_transfer_fail()
* ast_publish_attended_transfer_bridge_merge()
* ast_publish_attended_transfer_threeway()
* ast_publish_attended_transfer_app()
* ast_publish_attended_transfer_link()
* ast_bridge_publish_enter()
* ast_bridge_publish_leave()
* ast_bridge_publish_blind_transfer()
These functions' documentation now have a precondition of having no channels or bridges locked.
This review seeks only to make sure that channels are properly locked prior to creating channel snapshots. A similar effort is likely needed to ensure that bridges are locked during bridge snapshot creation.</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;">As stated in the description, I ran the atxfer_threeway_nominal test in a loop. After approximately 450 test runs, there were no negative consequences. Prior to this changeset, running the test 20-50 times would result in a crash.</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>/branches/12/tests/test_stasis_channels.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/tests/test_cel.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/tests/test_cdr.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/res/res_stasis.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/res/res_pjsip_refer.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/res/res_agi.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/res/parking/parking_manager.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/res/parking/parking_bridge_features.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/res/ari/resource_bridges.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/pbx/pbx_realtime.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/main/stasis_channels.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/main/stasis_bridges.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/main/pickup.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/main/pbx.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/main/endpoints.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/main/dial.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/main/core_unreal.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/main/core_local.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/main/channel.c <span style="color: grey">(402769)</span></li>
<li>/branches/12/main/cel.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/main/bridge_channel.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/main/bridge.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/include/asterisk/stasis_channels.h <span style="color: grey">(402686)</span></li>
<li>/branches/12/include/asterisk/stasis_bridges.h <span style="color: grey">(402686)</span></li>
<li>/branches/12/include/asterisk/channelstate.h <span style="color: grey">(402686)</span></li>
<li>/branches/12/include/asterisk/channel.h <span style="color: grey">(402686)</span></li>
<li>/branches/12/include/asterisk/aoc.h <span style="color: grey">(402686)</span></li>
<li>/branches/12/funcs/func_timeout.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/channels/sig_pri.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/channels/sig_analog.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/channels/chan_vpb.cc <span style="color: grey">(402686)</span></li>
<li>/branches/12/channels/chan_unistim.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/channels/chan_skinny.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/channels/chan_sip.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/channels/chan_pjsip.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/channels/chan_phone.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/channels/chan_oss.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/channels/chan_nbs.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/channels/chan_motif.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/channels/chan_misdn.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/channels/chan_mgcp.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/channels/chan_jingle.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/channels/chan_iax2.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/channels/chan_h323.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/channels/chan_gtalk.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/channels/chan_dahdi.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/channels/chan_console.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/channels/chan_alsa.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/apps/app_voicemail.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/apps/app_userevent.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/apps/app_queue.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/apps/app_meetme.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/apps/app_disa.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/apps/app_dial.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/apps/app_confbridge.c <span style="color: grey">(402687)</span></li>
<li>/branches/12/apps/app_agent_pool.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/addons/chan_ooh323.c <span style="color: grey">(402686)</span></li>
<li>/branches/12/addons/chan_mobile.c <span style="color: grey">(402686)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2961/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>