[asterisk-dev] [Code Review] 4160: chan_sip: Fix theoretical leak of p->refer

[Mark Michelson]

> On Nov. 11, 2014, 6:43 p.m., Mark Michelson wrote:
> > If I understand the purpose of p->refer correctly, it's supposed to be details relating to a specific REFER (or REFER-esque in some cases) transaction. I think that any in-dialog places where p->refer may be allocated, the previous p->refer should be freed. In addition to the transmit_refer() change you have, this would mean that handle_request_refer() and get_also_info() should free p->refer and then allocate a new one.
> > 
> > Honestly, the best way to do this is perhaps to just have sip_refer_alloc() destroy the old p->refer and then allocate a new one.
> 
> Corey Farrell wrote:
>     This seams reasonable to me.  Now that this will change existing behaviour, I want to run it through testsuite tests/channels/SIP.  Once that passes I'll update the review.  Unless I'm misunderstanding, the call to sip_refer_alloc from handle_request_invite should destroy any existing p->refer as well?

It will only happen with INVITEs that start a dialog, so p->refer will be NULL at that point. sip_refer_destroy() will be a no-op, so it's fine. If we get a reinvite with Replaces, Asterisk will send a 400 response before reaching the allocation of p->refer.


- Mark


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


On Nov. 10, 2014, 6:25 a.m., Corey Farrell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4160/
> -----------------------------------------------------------
> 
> (Updated Nov. 10, 2014, 6:25 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-15242
>     https://issues.asterisk.org/jira/browse/ASTERISK-15242
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> If transmit_refer is called when p->refer is already allocated, it will leak the previous allocation.  I checked for all occurrences of sip_refer_alloc, found that transmit_refer was the only caller that didn't check p->refer first.  This change moves the check for !p->refer to sip_refer_alloc.
> 
> I made transmit_refer destroy any previous p->refer so it will have a clean structure after reallocation like it does currently.  Unsure if it's needed, but the little bit of extra processing is worth keeping this fix low risk.
> 
> The change is slightly different in 12+, as p->refer->refer_call only exists in 11.
> 
> 
> Diffs
> -----
> 
>   /branches/11/channels/chan_sip.c 427666 
> 
> Diff: https://reviewboard.asterisk.org/r/4160/diff/
> 
> 
> Testing
> -------
> 
> Compiled, visual inspection.
> 
> 
> Thanks,
> 
> Corey Farrell
> 
>

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