[asterisk-commits] rmudgett: trunk r343579 - in /trunk: ./ channels/chan_sip.c

[SVN commits to the Asterisk project]

Author: rmudgett
Date: Mon Nov  7 13:54:09 2011
New Revision: 343579

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=343579
Log:
Fix deadlock if peer is destroyed while sending MWI notice.

A dialog cannot be destroyed by the ao2_callback dialog_needdestroy
because of a deadlock between the dialogs container lock and the RWLOCK of
the events subscription list.

* Create dialogs_to_destroy container to hold dialogs that will be
destroyed.

* Ensure that the event subscription callback will never happen with an
invalid peer pointer by making the event callback removal the first thing
in the peer destructor callback.

NOTE: This particular deadlock will not happen with Asterisk 10, but some
of the changes still apply.

(closes issue ASTERISK-18747)
Reported by: Gregory Hinton Nietsky

Review: https://reviewboard.asterisk.org/r/1564/
........

Merged revisions 343577 from http://svn.asterisk.org/svn/asterisk/branches/1.8
........

Merged revisions 343578 from http://svn.asterisk.org/svn/asterisk/branches/10

Modified:
    trunk/   (props changed)
    trunk/channels/chan_sip.c

Propchange: trunk/
------------------------------------------------------------------------------
Binary property 'branch-10-merged' - no diff available.

Modified: trunk/channels/chan_sip.c
URL: http://svnview.digium.com/svn/asterisk/trunk/channels/chan_sip.c?view=diff&rev=343579&r1=343578&r2=343579
==============================================================================
--- trunk/channels/chan_sip.c (original)
+++ trunk/channels/chan_sip.c Mon Nov  7 13:54:09 2011
@@ -2956,6 +2956,13 @@
 	return NULL;
 }
 
+/*!
+ * \brief Unlink a dialog from the dialogs container, as well as any other places
+ * that it may be currently stored.
+ *
+ * \note A reference to the dialog must be held before calling this function, and this
+ * function does not release that reference.
+ */
 void dialog_unlink_all(struct sip_pvt *dialog)
 {
 	struct sip_pkt *cp;
@@ -4589,9 +4596,18 @@
 static void sip_destroy_peer(struct sip_peer *peer)
 {
 	ast_debug(3, "Destroying SIP peer %s\n", peer->name);
-	if (peer->outboundproxy)
+
+	/*
+	 * Remove any mailbox event subscriptions for this peer before
+	 * we destroy anything.  An event subscription callback may be
+	 * happening right now.
+	 */
+	clear_peer_mailboxes(peer);
+
+	if (peer->outboundproxy) {
 		ao2_ref(peer->outboundproxy, -1);
-	peer->outboundproxy = NULL;
+		peer->outboundproxy = NULL;
+	}
 
 	/* Delete it, it needs to disappear */
 	if (peer->call) {
@@ -4625,7 +4641,6 @@
 	}
 	if (peer->dnsmgr)
 		ast_dnsmgr_release(peer->dnsmgr);
-	clear_peer_mailboxes(peer);
 
 	if (peer->socket.tcptls_session) {
 		ao2_ref(peer->socket.tcptls_session, -1);
@@ -17045,14 +17060,12 @@
  * \brief Match dialogs that need to be destroyed
  *
  * \details This is used with ao2_callback to unlink/delete all dialogs that
- * are marked needdestroy. It will return CMP_MATCH for candidates, and they
- * will be unlinked.
+ * are marked needdestroy.
  *
  * \todo Re-work this to improve efficiency.  Currently, this function is called
  * on _every_ dialog after processing _every_ incoming SIP/UDP packet, or
  * potentially even more often when the scheduler has entries to run.
  */
-
 static int dialog_needdestroy(void *dialogobj, void *arg, int flags)
 {
 	struct sip_pvt *dialog = dialogobj;