[asterisk-dev] [Code Review] Fix deadlock between subscription event RWLOCK and dialogs container lock in chan_sip. (simplified)

[David Vossel]

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



/branches/1.8/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1564/#comment8872>

    This is fine since we know that the iterator is not malloced when we use iterator_init on a stack allocated iterator, but why do we need to use an iterator at all?  What is the advantage of the iterator over a callback that does the exact same thing with OBJ_NODATA | OBJ_MULTIPLE | OBJ_UNLINK used?  Looking at the ao2_iterator_next() function, it might be more expensive than the callback when bucket collisions exist.
    
    
    I understand we don't hold the dialogs_needdestroy lock during the iterator traversal, but holding that lock won't cause a problem in this case.  It would actually be more efficient to hold the lock for the entire traversal instead of grabbing it and letting it go for each iteration.  Actually, since the dialog_needdestroy linking and unlinking exists in the same thread, we can just pass the iterator the NO_LOCK flag.  No synchronization is necessary... at least for now that is.
    


- David


On Nov. 3, 2011, 6:29 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1564/
> -----------------------------------------------------------
> 
> (Updated Nov. 3, 2011, 6:29 p.m.)
> 
> 
> Review request for Asterisk Developers, David Vossel and schmidts.
> 
> 
> Summary
> -------
> 
> Timing between dialog destruction and a MWI event sending a message could result in a deadlock.
> 
> Order of events causing deadlock:
> 
> 1a) The event subscription system calls the registered callbacks with its list RWLOCK held.
> 1b) The SIP monitor checks for dialogs needing destruction.  It does an ao2_callback that holds the dialogs container lock while searching for dialogs to destroy.
> 2a) The event subscription SIP callback needs to create a temporary dialog to send out the MWI notification.  That temporary dialog needs to be inserted in the dialogs container so it must wait.
> 2b) The dialog search finds a dialog to destroy and as a result releases the last reference for a peer.  The peer destructor attempts to get the subscription RWLOCK but must wait.
> 3) deadlock
> 
> 
> This is based off of the original diff posted for review: https://reviewboard.asterisk.org/r/1557/
> 
> It directly addresses the comment dvossel posted to the original diff of https://reviewboard.asterisk.org/r/1557/
> 
> 
> This addresses bug ASTERISK-18747.
>     https://issues.asterisk.org/jira/browse/ASTERISK-18747
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_sip.c 343338 
> 
> Diff: https://reviewboard.asterisk.org/r/1564/diff
> 
> 
> Testing
> -------
> 
> It compiles. :)
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20111104/3edf8787/attachment-0001.htm>