[asterisk-dev] [Code Review] 2663: Fix race condition in basic bridge when multiple parties leave a multi-party bridge

[rmudgett]

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

Ship it!


Make this minor fix and ship it.


/trunk/main/bridging_basic.c
<https://reviewboard.asterisk.org/r/2663/#comment17937>

    This should be (2 <= count) since the count is the number of remaining channels in the bridge after this one leaves.


- rmudgett


On July 9, 2013, 3:43 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2663/
> -----------------------------------------------------------
> 
> (Updated July 9, 2013, 3:43 p.m.)
> 
> 
> Review request for Asterisk Developers and rmudgett.
> 
> 
> Bugs: ASTERISK-21882
>     https://issues.asterisk.org/jira/browse/ASTERISK-21882
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This fixes a race condition in the basic bridge that could result in the bridge not being dissolved as expected if all but one party leaves the bridge simultaneously.
> 
> Prior to this change, a hangup hook in the basic bridge would check the number of channels remaining in the bridge and use that number in order to determine what state to set the hanging-up bridge channel to. If there were two or fewer participants remaining, then the hangup hook would do nothing. This would result in the default state of AST_BRIDGE_CHANNEL_STATE_END being set on the leaving bridge channel. Since bridge_basic has the AST_BRIDGE_FLAG_DISSOLVE_HANGUP flag set, it means that if a bridge channel leaves the bridge with AST_BRIDGE_CHANNEL_STATE_END, then the bridge will dissolve. The race condition comes into play during the hangup hook. The problem is that the number of channels in the bridge is not decremented until the bridge channel is pulled from the bridge, which happens after hangup hooks are run. This means that if multiple participants were to hang up simultaneously, leaving a single party in the bridge, it could result in all hangup hooks thinking that there were still more than two channels remaining in the bridge. The result would be that a lone participant would remain in the bridge.
> 
> The fix presented here comes from my realization that AST_BRIDGE_FLAG_DISSOLVE_HANGUP is not really a good fit for the basic bridge, because we really only want to dissolve the bridge when a channel hangs up under specific circumstances (i.e. there are two or fewer participants remaining in the bridge). Rather than having a hangup hook set the wheels in motion for having the bridge dissolve, I decided that a better idea would be to trigger bridge dissolution from a pull v_table method in the basic bridge. The race condition is resolved because when the pull method is called, the bridge is locked and the number of channels remaining in the bridge has been decremented. This way, each participant that is pulled from the bridge can be guaranteed to have an accurate count of the remaining parties in the bridge. If, after a pull, there are fewer than two parties remaining in the bridge, the bridge channel that is being pulled will have the AST_BRIDGE_CHANNEL_FLAG_DISSOLVE_HANGUP flag set on it. This will cause the bridge to dissolve.
> 
> 
> Diffs
> -----
> 
>   /trunk/main/bridging_basic.c 393881 
> 
> Diff: https://reviewboard.asterisk.org/r/2663/diff/
> 
> 
> Testing
> -------
> 
> It's difficult to test whether a race condition can still occur or not. This holds especially true in this case where the race condition, as far as I know, was never actually encountered by anyone but was recognized as possible from reading code.
> 
> Instead of trying to see if the race condition would still occur, I focused testing on ensuring that typical operation of a basic bridge still worked as expected. I performed a simple two-party call and had a party hang up. I ensured that the bridge dissolved as expected. I also tested an AMI redirect of a channel in a bridge and observed that the bridge dissolved.
> 
> 
> Thanks,
> 
> Mark Michelson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130709/1065d348/attachment-0001.htm>