[asterisk-dev] [Code Review] SIP: INVITE w/Replaces deadlock fix

[Olle E Johansson]

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

Ship it!


THis is a much needed cleanup of this code. I really appreciate you getting this fixed. As far as I can see, it looks very good and retains the logic, while still fixing all the locking and unlocking. Thank you! 
Someone else needs to verify the locking stuff... As the original author of this code, I'm disqualified by default   :-)

- Olle E


On 2009-09-17 14:48:19, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/371/
> -----------------------------------------------------------
> 
> (Updated 2009-09-17 14:48:19)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch cleans up the locking logic in chan_sip.c's handle_invite_replaces() function as well as making use of ast_do_masquerade() rather than forcing the masquerade on an ast_read().  The code had several redundant unlocks that would result in 'freed more times than we've locked!' errors. I cleaned these up as well as moving all the unlock logic to the end of the function.  This patch should also resolve the issue people were having with the replacecall channel never being unlocked with one legged calls.
> 
> This is very tricky code, as far as I can tell through my testing and tracing back through the code, the function starts with 4 locks held, the the sip_pvt, it's channel, and the replace sip_pvt and it's channel.  The function should end with only the original sip_pvt being locked as it will be unlocked by sipsock_read().
> 
> 
> This addresses bug 0015151.
>     https://issues.asterisk.org/view.php?id=0015151
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/channels/chan_sip.c 218511 
> 
> Diff: https://reviewboard.asterisk.org/r/371/diff
> 
> 
> Testing
> -------
> 
> Tested attended transfers using Replaces header.  Tried both waiting for transfer to pickup and then hanging up, as well as hanging up while transfer was still ringing.  Both work. no more mutex unlock errors.
> 
> 
> Thanks,
> 
> David
> 
>