[asterisk-commits] dvossel: branch dvossel/masq_locking_order_trunk r222150 - in /team/dvossel/m...

SVN commits to the Asterisk project asterisk-commits at lists.digium.com
Mon Oct 5 16:44:48 CDT 2009


Author: dvossel
Date: Mon Oct  5 16:44:45 2009
New Revision: 222150

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=222150
Log:
Fixes a race condition where do_masquerade could be called twice
for the same channel.


Modified:
    team/dvossel/masq_locking_order_trunk/channels/chan_sip.c
    team/dvossel/masq_locking_order_trunk/main/channel.c

Modified: team/dvossel/masq_locking_order_trunk/channels/chan_sip.c
URL: http://svnview.digium.com/svn/asterisk/team/dvossel/masq_locking_order_trunk/channels/chan_sip.c?view=diff&rev=222150&r1=222149&r2=222150
==============================================================================
--- team/dvossel/masq_locking_order_trunk/channels/chan_sip.c (original)
+++ team/dvossel/masq_locking_order_trunk/channels/chan_sip.c Mon Oct  5 16:44:45 2009
@@ -19696,8 +19696,6 @@
 	struct ast_channel *replacecall = p->refer->refer_call->owner;	/* The channel we're about to take over */
 	struct ast_channel *targetcall;		/* The bridge to the take-over target */
 
-	struct ast_channel *test;
-
 	/* Check if we're in ring state */
 	if (replacecall->_state == AST_STATE_RING)
 		earlyreplace = 1;
@@ -19724,8 +19722,8 @@
 			call we are replacing exists, so an accepted
 			can't harm */
 		transmit_response_with_sdp(p, "200 OK", req, XMIT_RELIABLE, FALSE, FALSE);
-		/* Do something more clever here */
 		ast_channel_unlock(c);
+		*nounlock = 1;
 		ast_channel_unlock(replacecall);
 		sip_pvt_unlock(p->refer->refer_call);
 		return 1;
@@ -19760,9 +19758,9 @@
 
 	/* Answer the incoming call and set channel to UP state */
 	transmit_response_with_sdp(p, "200 OK", req, XMIT_RELIABLE, FALSE, FALSE);
-		
+
 	ast_setstate(c, AST_STATE_UP);
-	
+
 	/* Stop music on hold and other generators */
 	ast_quiet_chan(replacecall);
 	ast_quiet_chan(targetcall);
@@ -19778,46 +19776,32 @@
 	else
 		ast_debug(4, "Invite/Replaces: Going to masquerade %s into %s\n", c->name, replacecall->name);
 
-	/* C should now be in place of replacecall. all channel locks should be removed before issuing the masq */
-	ast_channel_unlock(replacecall);
-	ast_channel_unlock(c);
-	if (ast_do_masquerade(replacecall)) {
-		ast_log(LOG_WARNING, "Failed to perform masquerade with INVITE replaces\n");
-	}
-	ast_channel_lock_both(replacecall, c);
-
-	if (earlyreplace || oneleggedreplace ) {
-		c->hangupcause = AST_CAUSE_SWITCH_CONGESTION;
-	}
-
-	ast_setstate(c, AST_STATE_DOWN);
-	ast_debug(4, "After transfer:----------------------------\n");
-	ast_debug(4, " -- C:        %s State %s\n", c->name, ast_state2str(c->_state));
-	if (replacecall)
-		ast_debug(4, " -- replacecall:        %s State %s\n", replacecall->name, ast_state2str(replacecall->_state));
-	if (p->owner) {
-		ast_debug(4, " -- P->owner: %s State %s\n", p->owner->name, ast_state2str(p->owner->_state));
-		test = ast_bridged_channel(p->owner);
-		if (test)
-			ast_debug(4, " -- Call bridged to P->owner: %s State %s\n", test->name, ast_state2str(test->_state));
-		else
-			ast_debug(4, " -- No call bridged to C->owner \n");
-	} else
-		ast_debug(4, " -- No channel yet \n");
-	ast_debug(4, "End After transfer:----------------------------\n");
-
-	/* unlock sip pvt and owner so hangup can do its thing */
+	/* C should now be in place of replacecall. all channel locks and pvt locks should be removed before issuing the masq */
 	ast_channel_unlock(replacecall);
 	ast_channel_unlock(c);
 	sip_pvt_unlock(p->refer->refer_call);
 	sip_pvt_unlock(p);
-	*nounlock = 1;
+	if (ast_do_masquerade(replacecall)) {
+		ast_log(LOG_WARNING, "Failed to perform masquerade with INVITE replaces\n");
+	}
+
+	ast_channel_lock(c);
+	if (earlyreplace || oneleggedreplace ) {
+		c->hangupcause = AST_CAUSE_SWITCH_CONGESTION;
+	}
+	ast_setstate(c, AST_STATE_DOWN);
+	ast_channel_unlock(c);
 
 	/* The call should be down with no ast_channel, so hang it up */
 	c->tech_pvt = dialog_unref(c->tech_pvt, "unref dialog c->tech_pvt");
+
+	/* c and c's tech pvt must be unlocked at this point for ast_hangup */
 	ast_hangup(c);
-	sip_pvt_lock(p); /* lock PVT structure again after hangup */
-
+
+	/* this indicates to handle_request_do that the owner channel has already been unlocked */
+	*nounlock = 1;
+	/* lock PVT structure again after hangup */
+	sip_pvt_lock(p);
 	return 0;
 }
 
@@ -21025,6 +21009,9 @@
 		transmit_notify_with_sipfrag(transferer, seqno, "486 Busy Here", TRUE);
 		append_history(transferer, "Xfer", "Refer failed");
 		ast_clear_flag(&transferer->flags[0], SIP_DEFER_BYE_ON_TRANSFER);
+		/* if transfer failed, go ahead and unlock targetcall_pvt and it's owner channel */
+		sip_pvt_unlock(targetcall_pvt);
+		ast_channel_unlock(target.chan1);
 	} else {
 		/* Transfer succeeded! */
 		const char *xfersound = pbx_builtin_getvar_helper(target.chan1, "ATTENDED_TRANSFER_COMPLETE_SOUND");
@@ -21042,17 +21029,19 @@
 
 		/* By forcing the masquerade, we know that target.chan1 and target.chan2 are bridged. We then
 		 * can queue connected line updates where they need to go.
+		 *
+		 * before a masquerade, all channel and pvt locks must be unlocked.  Any recursive
+		 * channel locks held before this function invalidates channel container locking order
 		 */
-		if (target.chan1->masq) {
-			/* before a masquerade, all channel locks must be unlocked.  Any recursive
-			 * channel locks held before this function invalidates channel container locking order */
-			ast_channel_unlock(target.chan1);
-			ast_channel_unlock(current->chan1); /* current.chan1 is p->owner before the masq, it was locked by socket_read() */
-			*nounlock = 1;  /* we just unlocked the dialog's channel and have no plans of locking it again. */
-			/* If the channel thread already did the masquerade, then we don't need to do anything */
-			ast_do_masquerade(target.chan1);
-			ast_channel_lock(target.chan1);
-		}
+		ast_channel_unlock(target.chan1);
+		ast_channel_unlock(current->chan1); /* current.chan1 is p->owner before the masq, it was locked by socket_read()*/
+		*nounlock = 1;  /* we just unlocked the dialog's channel and have no plans of locking it again. */
+		sip_pvt_unlock(targetcall_pvt);
+		sip_pvt_unlock(transferer);
+
+		ast_do_masquerade(target.chan1);
+
+		ast_channel_lock(transferer); /* the transferer pvt is expected to remain locked on return */
 
 		if (target.chan2) {
 			ast_channel_queue_connected_line_update(target.chan1, &connected_to_transferee);
@@ -21066,9 +21055,8 @@
 			ast_channel_update_connected_line(target.chan1, &connected_to_target);
 		}
 	}
-	/* remove locks from targetcall_pvt and target.chan1 (targetcall_pvt's owner at the beginning of the function) */
-	sip_pvt_unlock(targetcall_pvt);
-	ast_channel_unlock(target.chan1);
+
+	/* at this point if the transfer is successful only the transferer pvt should be locked. */
 	ast_party_connected_line_free(&connected_to_target);
 	ast_party_connected_line_free(&connected_to_transferee);
 	if (targetcall_pvt)

Modified: team/dvossel/masq_locking_order_trunk/main/channel.c
URL: http://svnview.digium.com/svn/asterisk/team/dvossel/masq_locking_order_trunk/main/channel.c?view=diff&rev=222150&r1=222149&r2=222150
==============================================================================
--- team/dvossel/masq_locking_order_trunk/main/channel.c (original)
+++ team/dvossel/masq_locking_order_trunk/main/channel.c Mon Oct  5 16:44:45 2009
@@ -3066,7 +3066,7 @@
 	struct ast_frame *f = NULL;	/* the return value */
 	int blah;
 	int prestate;
-	int count = 0, cause = 0;
+	int cause = 0;
 
 	/* this function is very long so make sure there is only one return
 	 * point at the end (there are only two exceptions to this).
@@ -5078,7 +5078,7 @@
 /*!
   \brief Masquerade a channel
 
-  \note Assumes _NO_ channels are locked.  If a channel is locked while calling
+  \note Assumes _NO_ channels and _NO_ channel pvt's are locked.  If a channel is locked while calling
         this function, it invalidates our channel container locking order.  All channels
 		must be unlocked before it is permissible to lock the channels' ao2 container.
 */
@@ -5095,7 +5095,7 @@
 		struct ast_party_connected_line connected;
 		struct ast_party_redirecting redirecting;
 	} exchange;
-	struct ast_channel *clonechan = original->masq;
+	struct ast_channel *clonechan;
 	struct ast_cdr *cdr;
 	int rformat = original->readformat;
 	int wformat = original->writeformat;
@@ -5109,12 +5109,38 @@
 	 * original channel's backend.  While the features are nice, which is the
 	 * reason we're keeping it, it's still awesomely weird. XXX */
 
+	ao2_lock(channels);
+
+	ast_channel_lock(original);
+
+	/* This checks to see if the masquerade has already happened or not.  There is a
+	 * race condition that exists for this function. Since all pvt and channel locks
+	 * must be let go before calling do_masquerade, it is possible that it could be
+	 * called multiple times for the same channel.  This check verifies whether
+	 * or not the masquerade has already been completed by another thread */
+	if (!original->masq) {
+		ast_channel_unlock(original);
+		ao2_unlock(channels);
+		return 0; /* masq already completed by another thread, or never needed to be done to begin with */
+	}
+
+	/* now that we have verified no race condition exists, set the clone channel */
+	clonechan = original->masq;
+
+	/* since this function already holds the global containers lock, unlocking original
+	 * for deadlock avoidance will not result in a sort of masquerade race condition */
+	while (ast_channel_trylock(clonechan)) {
+		CHANNEL_DEADLOCK_AVOIDANCE(original);
+	}
+
+	/* clear the masquerade channels */
+	original->masq = NULL;
+	clonechan->masqr = NULL;
+
+	/* unlink from channels container as name (which is the hash value) will change */
 	ao2_unlink(channels, original);
 	ao2_unlink(channels, clonechan);
 
-	/* Now that we have unlinked from the channels container, We need both
-	 * the original and clone channels locked */
-	ast_channel_lock_both(original, clonechan);
 	ast_debug(4, "Actually Masquerading %s(%d) into the structure of %s(%d)\n",
 		clonechan->name, clonechan->_state, original->name, original->_state);
 
@@ -5125,10 +5151,6 @@
 	   one */
 	free_translation(clonechan);
 	free_translation(original);
-
-	/* Unlink the masquerade */
-	original->masq = NULL;
-	clonechan->masqr = NULL;
 
 	/* Save the original name */
 	ast_copy_string(orig, original->name, sizeof(orig));
@@ -5372,7 +5394,6 @@
 		pthread_kill(original->blocker, SIGURG);
 	ast_debug(1, "Done Masquerading %s (%d)\n", original->name, original->_state);
 
-
 done:
 	/* it is possible for the clone channel to disappear during this */
 	if (clonechan) {
@@ -5381,7 +5402,7 @@
 	}
 	ast_channel_unlock(original);
 	ao2_link(channels, original);
-
+	ao2_unlock(channels);
 	return 0;
 }
 




More information about the asterisk-commits mailing list