[asterisk-dev] [Code Review] 3256: res_rtp_asterisk: Undo regression from ASTERISK-23213 while working around PJNATH assertion abort from ASTERISK-22911

[Joshua Colp]

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



/branches/11/res/res_rtp_asterisk.c
<https://reviewboard.asterisk.org/r/3256/#comment20611>

    There's actually a bug here you didn't cause, but can fix!
    
    If the component is RTP then rtp->s should be used, if the component is RTCP then rtp->rtcp->s should be used.
    
    Otherwise looks good!


- Joshua Colp


On Feb. 27, 2014, 4:38 p.m., Jonathan Rose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3256/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2014, 4:38 p.m.)
> 
> 
> Review request for Asterisk Developers, Joshua Colp, Kevin Harwell, and Matt Jordan.
> 
> 
> Bugs: ASTERISK-22911 and ASTERISK-23213
>     https://issues.asterisk.org/jira/browse/ASTERISK-22911
>     https://issues.asterisk.org/jira/browse/ASTERISK-23213
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Let me start by saying this is almost certainly not the complete answer to solving these problems. This patch is simply an alternative to backing out the patch from r405234 and leaving the existing aborts in place. I've written a test to make sure the new patch (and likely a later patch which can resolve these problems with ICE more comprehensively) does not crash Asterisk and that can be view here: https://reviewboard.asterisk.org/r/3255/
> 
> In my reproduction of this regression, I noticed a few things. The first was that when starting a call with ICE from SIPML5 that Asterisk would not be able to full initialize the ICE session. Instead when creating the candidate checklist via pj_ice_sess_create_check_list, PJNATH would be unable to associate the srflx candidates with any host pairs when pruning the checklist. The SRFLX candidates would have the addresses used internally on my LAN while the addresses it would be matched up against would mirror those of my external IP (in other words, they just didn't match by address). The overall return from pj_ice_sess_create_check_list would be PJNATH_EICENOHOSTCAND and while the ICE session wouldn't get flagged as started, I still continued to receive two way audio on both ends in Asterisk 11.7
> 
> Asterisk 11.8 however would clear the candidate lists at this point and I would get one way audio instead.
> 
> The crash in 11.7 from holding was caused because upon holding the call, the SDP would contain the same list of ICE candidates, so Asterisk would attempt to start the ICE session again. This time when it entered the create check list function, the maximum ICE candidates in the session would be exhausted causing PJNATH to assert and abort (which visibly appears more or less the same as a crash). This work around patch checks to see if a create checklist call will cause that value to expand above the maximum and if it would, we abandon starting ICE back up and we clear the current candidate list on the ICE session. In my own tests this seemed to work quite well (I had two way audio, Asterisk didn't terminate suddenly, and I was able to hold and resume the call while still maintaining two way audio and music on hold for all expected ends). According to Vytis though, neither this patch nor the original patch to resolve ASTERISK-22911 by kharwell actually fixed his audio problems anyway. Kharwell's patch fixed the assertions that were occurring because the candidate list would be cleared on any error including the PJNATH_EICENOHOSTCAND which most of the people in ASTERISK-23213 were most likely experiencing as well. I draw that conclusion because when they used this patch they reported that it fixed their issues without introducing any new crashes.
> 
> At this point, I'm not entirely sure that audio actually should be allowed to work when building the ICE session check list fails like it has been in this scenario. But we need to finalize a new Asterisk 11 release soon and we can't have this apparent regression in when we do so. In the future we need to find out more comprehensively why pj_ice_sess_create_check_list is failing and how to fix that.
> 
> 
> Props to Lott Caskey for providing some improvements to the patch that I wrote.
> 
> 
> Diffs
> -----
> 
>   /branches/11/res/res_rtp_asterisk.c 408284 
> 
> Diff: https://reviewboard.asterisk.org/r/3256/diff/
> 
> 
> Testing
> -------
> 
> Tested calls and holds with a SIPML5 call to a desk phone. Confirmed two way audio and music on hold.
> Requested that the reporter and participants of ASTERISK-23213 test the patch. Everyone who tested it confirmed that it fixed their problems.
> Ran https://reviewboard.asterisk.org/r/3255/ and checked the log files to see what was happening
> 
> 
> Thanks,
> 
> Jonathan Rose
> 
>

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