[asterisk-dev] [Code Review] 3603: func_jitterbuffer: fix errors and leaks caused by certain masquerade's

[Joshua Colp]

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



/branches/11/main/channel.c
<https://reviewboard.asterisk.org/r/3603/#comment22129>

    This change is hazardous. It makes it so that some file descriptors on the "original" channel which were from the past channel that inhabited the structure will survive if the new channel taking its place does not have those file descriptors set. Effectively the file descriptors on the channel become a combination of the both of them.
    
    In the case of a generator this is correct because the channel taking its place should have the same generator as before (if you get masqueraded in you should hear ringing or music on hold).
    
    The question becomes for this - what is the proper behavior? That of generators or that of everything else? If I masquerade into another channel should I then be using the jitterbuffer dialplan function that they invoked? If so should it be reset since there is a new source of media?
    
    Right now this doesn't work because that behavior is undefined. The jitterbuffer code itself assumes yes, but as you've said the file descriptor is not preserved so it can't.


- Joshua Colp


On June 10, 2014, 12:12 a.m., Corey Farrell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3603/
> -----------------------------------------------------------
> 
> (Updated June 10, 2014, 12:12 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-22409
>     https://issues.asterisk.org/jira/browse/ASTERISK-22409
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> During masquerade it is possible for the AST_JITTERBUFFER_FD to be cleared (set to -1).  This change adds a check when copying channel fd's to prevent clearing an FD with -1.  This seems to resolve the bad audio quality experienced after the masquerade.  When AST_JITTERBUFFER_FD was set to -1, this prevented the channel from polling that timer.  This caused RTP packets to be received late, and discarded.
> 
> The changes to func_jitterbuffer.c were created first.  ast_free(jbframe); is needed to prevent jbframe from leaking if it is rejected by jb_impl.  This ensures we don't start leaking packets if they are received too late or rejected by jb_impl for any other reason.
> 
> The second change to func_jitterbuffer prevents a leak of ast_null_frame's that were duplicated (ie with ast_frdup or ast_frisolate).  I believe this leak might actually be unrelated to the masquerade issue, and likely occurs for every single ast_null_frame.
> 
> 
> Diffs
> -----
> 
>   /branches/11/main/channel.c 415593 
>   /branches/11/funcs/func_jitterbuffer.c 415593 
> 
> Diff: https://reviewboard.asterisk.org/r/3603/diff/
> 
> 
> Testing
> -------
> 
> Verified the scenario outlined in ASTERISK-22409 no longer experiences audio quality loss, and no longer causes leaks (tested under valgrind).  I patched asterisk to ensure that ast_frfree performed an immediate free to ensure valgrind would report any attempted use after free.
> 
> In early testing, I used debug messages instead of the added ast_frfree's - I verified the leaked frames reported by valgrind matched exactly to the number of debug messages.
> 
> For the masquerade fix I tested with some debug code that showed the old and new FD, this is how I found the valid FD being replaced by -1.  See JIRA ticket for example output.
> 
> I have not tested this issue or fix against 12+, but the relevant code is the same as 11 - func_jitterbuffer code was moved to core but still the same code.
> 
> 
> Thanks,
> 
> Corey Farrell
> 
>

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