[asterisk-dev] [Code Review] 2899: Fix assumption of number of channels in native RTP bridges

[Matt Jordan]

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

Ship it!


Ship It!

- Matt Jordan


On Oct. 2, 2013, 6:57 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2899/
> -----------------------------------------------------------
> 
> (Updated Oct. 2, 2013, 6:57 p.m.)
> 
> 
> Review request for Asterisk Developers and Joshua Colp.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> The SIP one-legged transfer test was causing a crash in the testsuite. After investigating, the crash started occurring because of a (legitimate) fix to stop an RTP instance ref leak. The fix of the leak exposed a problem where an RTP instance's bridged pointer was pointing to a destroyed RTP instance.
> 
> After debugging, the problem became apparent. In the one-legged transfer test, two channels were in a native RTP local bridge. A third channel then replaced one of the channels in the bridge. The operation to replace a channel consists first of adding the new channel into the bridge and then removing the channel that is being replaced. The issue arose because the native RTP bridge code assumed that when its leave() callback was called, that there would be at most two channels in the bridge. As such, it would grab the first and last channels in the list of bridge channels and set their RTP instance bridged pointers NULL. As it turns out, the first and last channels in the bridge channel list were the first channel and the new third channel. As such, the second channel's RTP instance bridged pointer was never set NULL. It was left pointing to the first channel's RTP instance. This meant that when the first channel was hung up, a crash would soon happen since the second channel's RTP instance bridged pointer was pointing to a destroyed RTP instance.
> 
> My initial fix was to change native RTP bridging to grab the first and second channels from the list of bridge channels instead of the first and last. Instead, I went with a more robust approach that instead uses the channel we know is leaving as a base. We then can find which channel the leaving channel was bridged to and stop native RTP bridging those two channels. This method will work no matter the order that channels are added to the bridge.
> 
> I also added a note to the ast_bridge_technology leave() callback to indicate that no assumptions should be made regarding the number of channels that are in the bridge.
> 
> 
> Diffs
> -----
> 
>   /trunk/bridges/bridge_native_rtp.c 400253 
>   /trunk/include/asterisk/bridge_technology.h 400253 
> 
> Diff: https://reviewboard.asterisk.org/r/2899/diff/
> 
> 
> Testing
> -------
> 
> The SIP one-legged transfer test now passes as expected.
> 
> 
> Thanks,
> 
> Mark Michelson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20131002/552e8bf9/attachment-0001.html>