[asterisk-dev] [Code Review] Make sure that the lock order for sip_pvt and sip_pvt->owner is allways right. This to prevent deadlocks.

[David Vossel]

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


This patch assumes that every time a pvt lock is held, the pvt->owner lock must be held as well.   While this is true for many instances, this is not always true.

- David


On 2011-04-13 09:39:20, astmiv wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1177/
> -----------------------------------------------------------
> 
> (Updated 2011-04-13 09:39:20)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> The order in which sip_pvt and channels are locked is important.
> For different reported dealock bugs it appears this locking order is not respected.
> After examing different "core show lock" files it appears that the deadlock always happens when one thread first locks the sip_pvt->owner channel and later tries a lock for the sip_pvt itself. Another thread first locks the sip_pvt and then tries to lock the sip_pvt->owner channel (this is wrong).
> 
> We could try to fix this wrong lock order but might be very complicated because they happen in two different threads.
> 
> This update/patch makes sure that when a sip_pvt is locked it's sip_pvt->owner is always locked before.
> 
> sip_pvt lock: Check if sip_pvt has owner. If so wait for lock on sip_pvt->owner and remember that the owner was locked by the sip_pvt lock. When owner is locked lock sip_pvt itself.
> sip_pvt unlock: Unlock the sip_pvt. If we have an owner and the sip_pvt_lock locked it unlock owner.
> sip_pvt trylock: Check if sip_pvt has owner. If so wait for lock on sip_pvt->owner and remember that the owner was locked by the sip_pvt lock. When owner is locked do a trylock for sip_pvt. If the trylock is unsuccessful unlock owner.
> 
> For this to work all right we also have to monitor the change of the sip_pvt->owner. I created a separate function for this and altered the code where necessary.
> 
> The diff is not clean yet and is only a working concept. 
> 
> 
> This addresses bugs 19108 and 19112.
>     https://issues.asterisk.org/view.php?id=19108
>     https://issues.asterisk.org/view.php?id=19112
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_sip.c 313525 
>   /branches/1.8/channels/sip/dialplan_functions.c 313525 
>   /branches/1.8/channels/sip/include/sip.h 313525 
>   /branches/1.8/channels/sip/include/sip_utils.h 313525 
> 
> Diff: https://reviewboard.asterisk.org/r/1177/diff
> 
> 
> Testing
> -------
> 
> I have it currently running on a test system, 1.8.3.2, for which I was able to reproduce issue 19108. With this patch the reported deadlock of issue 19108 is history.
> I had to increase AST_MAX_LOCKS from 64 to 300 because one thread could not set any locks because it reached the limit.
> Currently still testing with sipp
> 
> 
> Thanks,
> 
> astmiv
> 
>

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