[asterisk-dev] [Code Review] Fix crashes in ast_rtcp_write()

[David Vossel]

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

Ship it!


A 5 second delayed destruction isn't that big of a deal.  I'm fine with this and completely agree that introducing any sort of synchronization here would just make this more complicated than is necessary.  Great Work!

- David


On Sept. 18, 2011, 9:20 p.m., Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1444/
> -----------------------------------------------------------
> 
> (Updated Sept. 18, 2011, 9:20 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> I opened ASTERISK-18570 for this issue.  The rest of the issues are ones that I have found so far that appear to be the same problem.
> 
> This patch addresses crashes related to RTCP handling.  The backtraces just show a crash in ast_rtcp_write() where it appears that the RTP instance is no longer valid.  There is a race condition with scheduled RTCP transmissions and the destruction of the RTP instance.  This patch utilizes the fact that ast_rtp_instance is a reference counted object and ensures that it will not get destroyed while a reference is still around due to scheduled RTCP transmissions.
> 
> RTCP transmissions are scheduled and executed from the chan_sip scheduler context.  This scheduler context is processed in the SIP monitor thread.  The destruction of an RTP instance occurs when the associated sip_pvt gets destroyed (which happens when the sip_pvt reference count reaches 0).  However, the SIP monitor thread is not the only thread that can cause a sip_pvt to get destroyed.  The sip_hangup function, executed from a channel thread, also decrements the reference count on a sip_pvt and could cause it to get destroyed.
> 
> While this is being changed anyway, the patch also removes calling ast_sched_del() from within the RTCP scheduler callback.  It's not helpful.  Simply returning 0 prevents the callback from being rescheduled.
> 
> 
> This addresses bugs ASTERISK-13334, ASTERISK-15257, ASTERISK-15406, ASTERISK-17560, ASTERISK-18570, ASTERISK-9716, and ASTERISK-9977.
>     https://issues.asterisk.org/jira/browse/ASTERISK-13334
>     https://issues.asterisk.org/jira/browse/ASTERISK-15257
>     https://issues.asterisk.org/jira/browse/ASTERISK-15406
>     https://issues.asterisk.org/jira/browse/ASTERISK-17560
>     https://issues.asterisk.org/jira/browse/ASTERISK-18570
>     https://issues.asterisk.org/jira/browse/ASTERISK-9716
>     https://issues.asterisk.org/jira/browse/ASTERISK-9977
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/res/res_rtp_asterisk.c 335789 
> 
> Diff: https://reviewboard.asterisk.org/r/1444/diff
> 
> 
> Testing
> -------
> 
> This patch has been applied to a test environment with a couple of servers running Asterisk 1.8.7.0-rc1.  The servers have processed over 1 million calls without hitting this crash.
> 
> 
> Thanks,
> 
> Russell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110919/a33c35e1/attachment.htm>