[asterisk-dev] [Code Review] SIP: deadlock in local_attended_transfer()

[David Vossel]

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

(Updated 2009-09-30 17:39:39.595118)


Review request for Asterisk Developers.


Changes
-------

Since this function ultimately returns back to socket_read(), we should unlock the pvt's owner before doing the masquerade as it may change.


Summary
-------

There is a deadlock occurring during chan_sip's local_attended_transfer() function.  It is caused by unlocking the channel we are about to masquerade right before the masquerade actually happens.  The fix is simple, move the unlock until after the masquerade, but I noticed a few other things in the function that are not correct and fixed them as well.

The local_attended_transfer() function starts with only 2 locks (pvt and pvt->owner) and should return with only these two locks held.  At the beginning of this function get_sip_pvt_byid_locked() returns with both the targetcall_pvt and it's owner channel locked... So by the end of the function the pvt and channel locked by the get_sip_pvt_byid_locked() function _MUST_ be unlocked.  At this point we have 4 locks.  After a masquerade the targetcall_pvt's owner is not the same channel that was locked at the beginning of the function, so unlocking targetcall_pvt->owner doesn't appear to make any sense.  At the beginning of the function the original pointer to targetcall_pvt->owner was saved into target.chan1, so after all is said and done regardless if the transfer fails or not the targetcall_pvt as well as target.chan1 should be unlocked before returning.

  


This addresses bug 15618.
    https://issues.asterisk.org/view.php?id=15618


Diffs (updated)
-----

  /trunk/channels/chan_sip.c 221477 

Diff: https://reviewboard.asterisk.org/r/382/diff


Testing
-------

I executed the code by issuing an attended transfer, verified no deadlocks occurred.


Thanks,

David