[asterisk-dev] [Code Review] 4302: bridge: channel ref leak after failed blond transfer

[Scott Griepentrog]

> On Dec. 31, 2014, 3:46 p.m., Matt Jordan wrote:
> > /branches/13/main/bridge_basic.c, lines 2398-2410
> > <https://reviewboard.asterisk.org/r/4302/diff/1/?file=70051#file70051line2398>
> >
> >     I'm not sure we should be clearing the reference here.
> >     
> >     Clearly, we need to clear the reference to props->recall_target if the channel answers. In that case, however, that could be done on line 2341.
> >     
> >     If, however, the target doesn't answer, then we may still have to do something with the recall_target. That, of course, depends on the current state that the state machine is in - but here, it looks like we've blown away any chance for the state machine to use the original recall_target reference, regardless of the outcome of the dial operation.
> >     
> >     When the leak occurs, what state is the state machine in? Are there states in the state machine that would like to reference the recall_target channel?
> >

After reviewing the state machine operation more closely, I've determined that recall_target is used only in the BLOND_NONFINAL, RECALLING, and RETRANSFER states.  In each state, the enter function presumes that recall_target is not being used, and is free to set it to the referenced target channel.  The exit function is then responsible for insuring that the reference is released, but this is only required if the next state is another recall, retransfer, or wait-to-recall/transfer.  Any transition to FAIL or RESUME ot other state will end up cleaning up the recall_target on props destruction.

Thus I've reworked this change to fix the leaked recall_target channel on blond_nonfinal_exit, to more properly match the state machine convention.


- Scott


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


On Jan. 2, 2015, 10:46 a.m., Scott Griepentrog wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4302/
> -----------------------------------------------------------
> 
> (Updated Jan. 2, 2015, 10:46 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24513
>     https://issues.asterisk.org/jira/browse/ASTERISK-24513
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> After a blond transfer to a destination that then hangs up, the Local;1 channel would suffer a reference leak.  This was supposed to be fixed by review 4262, but that caused an extra unreference on nominal blond transfer (somehow missed that in testing).  This correction to the original problem reverts the prior change which had incorrectly mucked with moving the referenced transfer_target channel to recall_target in blond_nonfinal_enter(), and instead resolves the issue by insuring that the reference in recall_target is not lost by releasing it on blond_final_exit() before it is later replaced by an answered recall dial.
> 
> 
> Diffs
> -----
> 
>   /branches/13/main/bridge_basic.c 430163 
> 
> Diff: https://reviewboard.asterisk.org/r/4302/diff/
> 
> 
> Testing
> -------
> 
> Both the blond_nominal and atxfer_fail_blond (r/4256) tests pass without error.
> 
> 
> Thanks,
> 
> Scott Griepentrog
> 
>

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