<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 />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 29th, 2013, 11:08 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/2961/diff/1/?file=47583#file47583line1569" style="color: black; font-weight: bold; text-decoration: underline;">/branches/12/main/pbx.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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 check_contexts(char *file, int line )</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1569</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">int pbx_exec(struct ast_channel *c,<span class="tb"> </span>/*!< Channel */</pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1569</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">int pbx_exec(struct ast_channel *c,<span class="tb"> </span>/*!< Channel */</pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1570</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span> struct ast_app *app,<span class="tb"> </span>/*!< Application */</pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1570</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span> struct ast_app *app,<span class="tb"> </span>/*!< Application */</pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1571</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span> const char *data)<span class="tb"> </span><span class="tb"> </span>/*!< Data for execution */</pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1571</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span> const char *data)<span class="tb"> </span><span class="tb"> </span>/*!< Data for execution */</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;">I do wonder about this functions channel snapshot. The channel snapshot is updated when the app is executed but the snapshot is not restored when the app completes. For normal dialplan execution, this is good. For the other cases where an app is executed for miscellaneous reasons, maybe the channel should get a snapshot when the app is restored on exit of pbx_exec().</pre>
</blockquote>
<p>On October 30th, 2013, 1:35 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;">If you remove the channel snapshot here, you *will* break a lot of things. If you publish an additional snapshot here, you will also - most likely, anyway - break things as well, as the resetting of the application/data will make it look like the channel just re-entered that application. This occurs most frequently with Originated and Dialed channels - but will also screw up other channels that execute subroutines.
If you decide to get adventurous and change this code, be prepared to have CDRs break in strange and creative ways.</pre>
</blockquote>
<p>On October 30th, 2013, 4:34 p.m. UTC, <b>rmudgett</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;">The first snapshot in this function is necessary. For normal dialplan execution, the app/data does not need to be restored because there shouldn't be any app/data so a second snapshot is not desireable. For interception routines or applications that execute other applications, a second snapshot for the restoration of the app/data seems to be needed.</pre>
</blockquote>
<p>On October 30th, 2013, 6:39 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;">"For interception routines or applications that execute other applications, a second snapshot for the restoration of the app/data seems to be needed."
It is not needed by any existing consumer of Stasis. In fact, 'restoring' the application after an intercept routine runs complicates and confuses a consumer's view of the world.
Say I'm a consumer of Stasis, watching messages about SIP/foo, and I care about what application SIP/foo happens to be in. When SIP/foo enters into an application, I make some logical decisions in my application.
Let's say I get the following application + CEP changes:
default,1234,1: NoOp
default,1234,2: Dial
default,my_routine,1: NoOp
default,my_routine,2: CONNECTEDLINE
default,my_routine,3: Return
default,1234,2: Dial
By re-sending the Dial application, you've required consumers of Stasis to maintain their own program counter for each channel so that they 'know' whether or not the second Dial application that they see is the same Dial application the channel was already in, or the existing Dial application. They have to "know" how Subroutine execution works, as well as Macro execution. You've increased the complexity and the difficulty of each consumer in maintaining application state.
If you don't send the second Dial, you do run the risk that the consumers don't 'know' when the channel returns into the Dial application. Thus far, that has been by far the lesser of these two evils. I do not advocate making this change unless there is an absolute clear necessity for it.</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;">Regardless of a second snapshot being taken or not by this routine, if a channel snapshot is taken later for whatever reason, the app/data has been restored.</pre>
<br />
<p>- rmudgett</p>
<br />
<p>On October 25th, 2013, 5:07 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 Oct. 25, 2013, 5:07 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">(401703)</span></li>
<li>/branches/12/tests/test_cel.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/tests/test_cdr.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/res/res_stasis.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/res/res_pjsip_refer.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/res/res_agi.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/res/parking/parking_manager.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/res/parking/parking_bridge_features.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/res/ari/resource_bridges.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/pbx/pbx_realtime.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/main/stasis_channels.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/main/stasis_bridges.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/main/pickup.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/main/pbx.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/main/endpoints.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/main/dial.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/main/core_unreal.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/main/core_local.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/main/channel.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/main/cel.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/main/bridge_channel.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/main/bridge.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/include/asterisk/stasis_channels.h <span style="color: grey">(401703)</span></li>
<li>/branches/12/include/asterisk/stasis_bridges.h <span style="color: grey">(401703)</span></li>
<li>/branches/12/include/asterisk/channelstate.h <span style="color: grey">(401703)</span></li>
<li>/branches/12/include/asterisk/channel.h <span style="color: grey">(401703)</span></li>
<li>/branches/12/include/asterisk/aoc.h <span style="color: grey">(401703)</span></li>
<li>/branches/12/funcs/func_timeout.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/channels/sig_pri.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/channels/sig_analog.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/channels/chan_vpb.cc <span style="color: grey">(401703)</span></li>
<li>/branches/12/channels/chan_unistim.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/channels/chan_skinny.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/channels/chan_sip.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/channels/chan_pjsip.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/channels/chan_phone.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/channels/chan_oss.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/channels/chan_nbs.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/channels/chan_motif.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/channels/chan_misdn.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/channels/chan_mgcp.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/channels/chan_jingle.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/channels/chan_iax2.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/channels/chan_h323.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/channels/chan_gtalk.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/channels/chan_dahdi.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/channels/chan_console.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/channels/chan_alsa.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/apps/app_voicemail.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/apps/app_userevent.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/apps/app_queue.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/apps/app_meetme.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/apps/app_disa.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/apps/app_dial.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/apps/app_confbridge.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/apps/app_agent_pool.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/addons/chan_ooh323.c <span style="color: grey">(401703)</span></li>
<li>/branches/12/addons/chan_mobile.c <span style="color: grey">(401703)</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>