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

[Corey Farrell]

> On June 10, 2014, 7:22 a.m., Joshua Colp wrote:
> > /branches/11/main/channel.c, line 7113
> > <https://reviewboard.asterisk.org/r/3603/diff/1/?file=59449#file59449line7113>
> >
> >     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.
> 
> Matt Jordan wrote:
>     There is another way to handle this that may be safer: the fixup callback on the jitterbuffer channel datastore.
>     
>     Inside that fixup, the timer on the old channel can be forgotten (or properly disposed of) and a new one created on the channel we are masquerading into (although some checks should probably be done to make sure that if a timer is already on it, we clean it up).

I feel that jitterbuffer should at minimum follow the same rules as a generator.  If I enable jitterbuffer on a Local channel then Dial(SIP/anything), I expect to have a functioning jitterbuffer after SIP/anything is answered, the same as if the local channel was non-optimizing.  On the other hand if I have jitterbuffer enabled by a variable set in sip.conf for the peer "anything", then I expect it to not be disabled by lack of jitterbuffer on the Local channel.  In situations where both sides of the masquerade have jitterbuffer enabled I think we should use the JB from 'original' instead of from 'clonechan'.  My thought is that JB was explicitly set by dialplan for 'original', where clonechan is set by peer configuration and is therefore just a default.


1) We can fix situations where JB is set on 'original' but not 'clonechan' by not overwriting AST_JITTERBUFFER_FD.  I feel like this should be dealt with the same way as AST_GENERATOR_FD.


2) For situations where both channels have JB set, we have to prevent the JB datastore of 'clonechan' from being copied to 'original'.  It looks like the datastore chan_fixup would need to free the 'clonechan' datastore when this happens.


3) I think any situation where 'clonechan' has JB set will currently cause corruption.  ast_do_masquerade transfers datastores but not framehooks.  This would cause the framehook of 'clonechan' to be freed, but the datastore will be transferred to 'original' with datastore->data pointing to an invalid 'int *id'.

I think this could be fixed by the datastore chan_fixup.  It could retrieve the framehook data from 'clonechan', copy all fields to a new structure and zero the framehook data from clonechan to prevent jb/timer from being destroyed when that framehook is freed.  Finally we would need to set AST_JITTERBUFFER_FD on 'original'.

For this to work we will need a new framehook.c function:
void *ast_framehook_get_data(struct ast_channel *chan, int framehook_id);


- Corey


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


On June 9, 2014, 8:12 p.m., Corey Farrell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3603/
> -----------------------------------------------------------
> 
> (Updated June 9, 2014, 8:12 p.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/1562c978/attachment-0001.html>