<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/3108/">https://reviewboard.asterisk.org/r/3108/</a>
</td>
</tr>
</table>
<br />
<table bgcolor="#e0e0e0" width="100%" cellpadding="8" style="border: 1px gray solid;">
<tr>
<td>
<h1 style="margin-right: 0.2em; padding: 0; font-size: 10pt;">This change has been marked as submitted.</h1>
</td>
</tr>
</table>
<br />
<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 Matt Jordan.</div>
<p style="color: grey;"><i>Updated Jan. 9, 2014, 4:40 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/AST-1258">AST-1258</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;">Say we have three users who are joining a ConfBridge: two wait marked (WM) users and a marked (M) user. Say the WM users join the conference and wait for awhile being entertained. The M user then joins, and the ConfBridge starts. Internally, this is represented by state transitions that also keep track of the number of active and waiting users. In this case, the ConfBridge state goes through the following:
1. WM1 joins EMPTY -> INACTIVE (active: 0; waiting: 1)
2. WM2 joins INACTIVE (active: 0; waiting: 2)
3. M1 joins INACTIVE -> MULTI_MARKED (active: 3; waiting: 0)
Now say someone - perhaps an overzealous sys admin - jumps onto Asterisk and starts booting users left and right. They even have a script! They issue the CLI kick user command so fast that we immediately kick M1 first, followed by WM1 and WM2. Ideally, this would do the following:
4. M1 leaves MULTI_MARKED -> INACTIVE (active: 0; waiting: 2)
5. WM2 leaves INACTIVE (waiting: 1)
6. WM1 leaves INACTIVE (waiting: 0)
When we transition states, the ConfBridge *must* be locked. This prevents the user counts from getting jumbled and the state transitions from being perturbed. However, MULTI_MARKED has a race condition: in order to play a prompt to the bridge notifying the WM users that the leader has left the conference, it unlocks the conference. This allows the WM user(s) to leave prior to the M1 user from completely leaving/transitioning the conference. More often than not, this causes a crash as ConfBridge enters into some very invalid states.
This patch fixes this by doing the following:
(1) It prevents a user from getting kicked more than once. This isn't strictly necessary, but is a good idea - we already keep track of whether or not a user has been kicked, and kicking them multiple times is risky.
(2) It removes the race condition by deferring the "leader has left" playback until after the conference state has transitioned. If someone sneaks in and leaves at that time; no harm, no foul.</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;">An automated test was written that reproduced the scenario. Without the patch: Explosions. With the patch: Asterisk allowed the users to be kicked without terminating itself.
Note that the tests will be posted under a separate review.</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/11/apps/confbridge/conf_state_multi_marked.c <span style="color: grey">(404856)</span></li>
<li>/branches/11/apps/app_confbridge.c <span style="color: grey">(404856)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3108/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>