[asterisk-dev] [Code Review]: chan_sip REFER deadlock fixes

[David Vossel]

> On Aug. 11, 2011, 2:34 p.m., rmudgett wrote:
> > /branches/1.8/channels/chan_sip.c, lines 22767-22769
> > <https://reviewboard.asterisk.org/r/1339/diff/2/?file=17704#file17704line22767>
> >
> >     Calling get_refer_info() with p and p->owner locked is probably not good.
> >     
> >     get_refer_info() calls pbx_builtin_setvar_helper() on the bridged peer of p and thus would need deadlock avoidance between p->owner and peer.

This is going to have to be handled in the get_refer_info function.  We'll need to establish the preconditions for get_refer_info in order to know how to properly unlock and relock the pvt and pvt->owner to account for this.


> On Aug. 11, 2011, 2:34 p.m., rmudgett wrote:
> > /branches/1.8/channels/chan_sip.c, line 22896
> > <https://reviewboard.asterisk.org/r/1339/diff/2/?file=17704#file17704line22896>
> >
> >     local_attended_transfer() calls get_sip_pvt_byid_locked().  Since p and p->owner are already locked, we can have a deadlock when get_sip_pvt_byid_locked() attempts to lock the referred dialog.  The deadlock potential is reduced by the deadlock avoidance done by get_sip_pvt_byid_locked() but not entirely eliminated since the locks already held could be wanted by some other transfer request.
> >     
> >     Reference leak in get_sip_pvt_byid_locked()!
> >     The sip_pvt_ptr is not unreffed for error exits it is only unlocked.

local_attended_transfer establishes the precondition for the function to have the sip pvt and pvt owner locked.  This is an issue that will have to be resolved in local_attended_transfer()


- David


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


On Aug. 2, 2011, 4:28 p.m., David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1339/
> -----------------------------------------------------------
> 
> (Updated Aug. 2, 2011, 4:28 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> handle_request_refer() is a complete mess when it comes to locking.  A deadlock occurs, we fix it, and then it moves somewhere else.  This patch attempts to resolve all the possible locking inversion issues that can occur in this function.
> 
> 
> This addresses bug ASTERISK-18082.
>     https://issues.asterisk.org/jira/browse/ASTERISK-18082
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_sip.c 330671 
> 
> Diff: https://reviewboard.asterisk.org/r/1339/diff
> 
> 
> Testing
> -------
> 
> I tested refer using a snom phone with blind transfer, but that is not very impressive.
> 
> James Van Vleet has tested this code using a load testing tool that was capable of exposing all sorts of problems.  He has reported that his test is running without issue using this iteration of the patch.  Given what it was capable of exposing earlier, I am confident in these test results.
> 
> 
> Thanks,
> 
> David
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110812/b00fd798/attachment.htm>