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

[rmudgett]

> On Nov. 3, 2011, 4:13 p.m., schmidts wrote:
> > /branches/1.8/channels/chan_sip.c, lines 25312-25320
> > <https://reviewboard.asterisk.org/r/1564/diff/1/?file=21604#file21604line25312>
> >
> >     i prefer move the iter block above the callback for the dialog container.
> >     with this you can make sure that there will not be a lock enabled before deleting the dialog. normally it takes between 1 ms to max. 1000 ms to come back here so this should be fast enough.
> >     
> >     btw i think checking if dialogs_needdestroy container is empty should be faster than an iterator_init to an empty container and this will happens most of the time.

I don't understand.
The iter block has to follow the ao2_callback of dialog_needdestroy.  The dialog_needdestroy() function puts dialogs into the dialogs_needdestroy container rather than destroying the dialogs immediately.  The following iter block then destroys all dialogs in the dialog_needdestroy container.

There are no locks held when dialog_unlink_all() is called here.  The ao2_iterator_next() only holds the container lock while it is finding the next object.

However, testing ao2_container_count before iterating would be faster.


- rmudgett


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


On Nov. 3, 2011, 3:14 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1564/
> -----------------------------------------------------------
> 
> (Updated Nov. 3, 2011, 3:14 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/20111103/555bbb5e/attachment.htm>