[asterisk-dev] [Code Review] 3108: confbridge: Prevent race condition leading to crash in state transitions during bridge announcement in MULTI_MARKED state

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3108/#review10525
-----------------------------------------------------------

Ship it!


Ship It!

- Mark Michelson


On Jan. 7, 2014, 6:33 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3108/
> -----------------------------------------------------------
> 
> (Updated Jan. 7, 2014, 6:33 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: AST-1258
>     https://issues.asterisk.org/jira/browse/AST-1258
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   /branches/11/apps/confbridge/conf_state_multi_marked.c 404856 
>   /branches/11/apps/app_confbridge.c 404856 
> 
> Diff: https://reviewboard.asterisk.org/r/3108/diff/
> 
> 
> Testing
> -------
> 
> 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.
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140108/5dc49e4d/attachment.html>