[asterisk-commits] rmudgett: branch 10 r368407 - in /branches/10: ./ main/channel.c

SVN commits to the Asterisk project asterisk-commits at lists.digium.com
Mon Jun 4 14:08:57 CDT 2012


Author: rmudgett
Date: Mon Jun  4 14:08:52 2012
New Revision: 368407

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=368407
Log:
Fix potential deadlock between masquerade and chan_local.

* Restructure ast_do_masquerade() to not hold channel locks while it calls
ast_indicate().

* Simplify many calls to ast_do_masquerade() since it will never return a
failure now.  If it does fail internally because a channel driver callback
operation failed, the only thing ast_do_masquerade() can do is generate a
warning message about strange things may happen and press on.

* Fixed the call to ast_bridged_channel() in ast_do_masquerade().  This
change fixes half of the deadlock reported in ASTERISK-19801 between
masquerades and chan_iax.

(closes issue ASTERISK-19537)
Reported by: rmudgett
Tested by: rmudgett

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

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

Modified:
    branches/10/   (props changed)
    branches/10/main/channel.c

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

Modified: branches/10/main/channel.c
URL: http://svnview.digium.com/svn/asterisk/branches/10/main/channel.c?view=diff&rev=368407&r1=368406&r2=368407
==============================================================================
--- branches/10/main/channel.c (original)
+++ branches/10/main/channel.c Mon Jun  4 14:08:52 2012
@@ -2812,13 +2812,7 @@
 	 */
 	while (chan->masq) {
 		ast_channel_unlock(chan);
-		if (ast_do_masquerade(chan)) {
-			ast_log(LOG_WARNING, "Failed to perform masquerade\n");
-
-			/* Abort the loop or we might never leave. */
-			ast_channel_lock(chan);
-			break;
-		}
+		ast_do_masquerade(chan);
 		ast_channel_lock(chan);
 	}
 
@@ -2834,6 +2828,7 @@
 		return 0;
 	}
 
+	/* Mark as a zombie so a masquerade cannot be setup on this channel. */
 	if (!(was_zombie = ast_test_flag(chan, AST_FLAG_ZOMBIE))) {
 		ast_set_flag(chan, AST_FLAG_ZOMBIE);
 	}
@@ -3206,10 +3201,8 @@
 
 	/* Perform any pending masquerades */
 	for (x = 0; x < n; x++) {
-		if (c[x]->masq && ast_do_masquerade(c[x])) {
-			ast_log(LOG_WARNING, "Masquerade failed\n");
-			*ms = -1;
-			return NULL;
+		while (c[x]->masq) {
+			ast_do_masquerade(c[x]);
 		}
 
 		ast_channel_lock(c[x]);
@@ -3340,10 +3333,8 @@
 
 
 	/* See if this channel needs to be masqueraded */
-	if (chan->masq && ast_do_masquerade(chan)) {
-		ast_log(LOG_WARNING, "Failed to perform masquerade on %s\n", chan->name);
-		*ms = -1;
-		return NULL;
+	while (chan->masq) {
+		ast_do_masquerade(chan);
 	}
 
 	ast_channel_lock(chan);
@@ -3422,10 +3413,8 @@
 	struct ast_channel *winner = NULL;
 
 	for (i = 0; i < n; i++) {
-		if (c[i]->masq && ast_do_masquerade(c[i])) {
-			ast_log(LOG_WARNING, "Masquerade failed\n");
-			*ms = -1;
-			return NULL;
+		while (c[i]->masq) {
+			ast_do_masquerade(c[i]);
 		}
 
 		ast_channel_lock(c[i]);
@@ -3804,11 +3793,8 @@
 	 */
 
 	if (chan->masq) {
-		if (ast_do_masquerade(chan))
-			ast_log(LOG_WARNING, "Failed to perform masquerade\n");
-		else
-			f =  &ast_null_frame;
-		return f;
+		ast_do_masquerade(chan);
+		return &ast_null_frame;
 	}
 
 	/* if here, no masq has happened, lock the channel and proceed */
@@ -4890,12 +4876,9 @@
 		goto done;
 
 	/* Handle any pending masquerades */
-	if (chan->masq) {
+	while (chan->masq) {
 		ast_channel_unlock(chan);
-		if (ast_do_masquerade(chan)) {
-			ast_log(LOG_WARNING, "Failed to perform masquerade\n");
-			return res; /* no need to goto done: chan is already unlocked for masq */
-		}
+		ast_do_masquerade(chan);
 		ast_channel_lock(chan);
 	}
 	if (chan->masqr) {
@@ -6644,10 +6627,11 @@
  */
 int ast_do_masquerade(struct ast_channel *original)
 {
-	int x, i;
-	int res=0;
+	int x;
+	int i;
 	int origstate;
 	int visible_indication;
+	int clone_was_zombie = 0;/*!< TRUE if the clonechan was a zombie before the masquerade. */
 	struct ast_frame *current;
 	const struct ast_channel_tech *t;
 	void *t_pvt;
@@ -6670,60 +6654,64 @@
 	char masqn[AST_CHANNEL_NAME];
 	char zombn[AST_CHANNEL_NAME];
 
-	ast_format_copy(&rformat, &original->readformat);
-	ast_format_copy(&wformat, &original->writeformat);
-
 	/* XXX This operation is a bit odd.  We're essentially putting the guts of
 	 * the clone channel into the original channel.  Start by killing off the
 	 * original channel's backend.  While the features are nice, which is the
 	 * reason we're keeping it, it's still awesomely weird. XXX */
 
-	/* The reasoning for the channels ao2_container lock here is complex.
-	 * 
-	 * In order to check for a race condition, the original channel must
-	 * be locked.  If it is determined that the masquerade should proceed
-	 * the original channel can absolutely not be unlocked until the end
-	 * of the function.  Since after determining the masquerade should
-	 * continue requires the channels to be unlinked from the ao2_container,
-	 * the container lock must be held first to achieve proper locking order.
+	/*
+	 * The reasoning for the channels ao2_container lock here is
+	 * complex.
+	 *
+	 * There is a race condition that exists for this function.
+	 * Since all pvt and channel locks must be let go before calling
+	 * ast_do_masquerade, it is possible that it could be called
+	 * multiple times for the same channel.  In order to prevent the
+	 * race condition with competing threads to do the masquerade
+	 * and new masquerade attempts, the channels container must be
+	 * locked for the entire masquerade.  The original and clonechan
+	 * need to be unlocked earlier to avoid potential deadlocks with
+	 * the chan_local deadlock avoidance method.
+	 *
+	 * The container lock blocks competing masquerade attempts from
+	 * starting as well as being necessary for proper locking order
+	 * because the channels must to be unlinked to change their
+	 * names.
+	 *
+	 * The original and clonechan locks must be held while the
+	 * channel contents are shuffled around for the masquerade.
+	 *
+	 * The masq and masqr pointers need to be left alone until the
+	 * masquerade has restabilized the channels to prevent another
+	 * masquerade request until the AST_FLAG_ZOMBIE can be set on
+	 * the clonechan.
 	 */
 	ao2_lock(channels);
 
-	/* lock the original channel to determine if the masquerade is required or not */
+	/*
+	 * Lock the original channel to determine if the masquerade is
+	 * still required.
+	 */
 	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.
-	 */
-	while ((clonechan = original->masq) && ast_channel_trylock(clonechan)) {
+	clonechan = original->masq;
+	if (!clonechan) {
 		/*
-		 * A masq is needed but we could not get the clonechan lock
-		 * immediately.  Since this function already holds the global
-		 * container lock, unlocking original for deadlock avoidance
-		 * will not result in any sort of masquerade race condition.  If
-		 * masq is called by a different thread while this happens, it
-		 * will be stuck waiting until we unlock the container.
+		 * The masq is already completed by another thread or never
+		 * needed to be done to begin with.
 		 */
-		CHANNEL_DEADLOCK_AVOIDANCE(original);
-	}
-
-	/*
-	 * A final masq check must be done after deadlock avoidance for
-	 * clonechan above or we could get a double masq.  This is
-	 * posible with ast_hangup at least.
-	 */
-	if (!clonechan) {
-		/* masq already completed by another thread, or never needed to be done to begin with */
 		ast_channel_unlock(original);
 		ao2_unlock(channels);
 		return 0;
 	}
+
+	/* Bump the refs to ensure that they won't dissapear on us. */
+	ast_channel_ref(original);
+	ast_channel_ref(clonechan);
+
+	/* unlink from channels container as name (which is the hash value) will change */
+	ao2_unlink(channels, original);
+	ao2_unlink(channels, clonechan);
 
 	/* Get any transfer masquerade connected line exchange data. */
 	xfer_ds = ast_channel_datastore_find(original, &xfer_ds_info, NULL);
@@ -6735,30 +6723,26 @@
 	}
 
 	/*
-	 * Release any hold on the transferee channel before proceeding
-	 * with the masquerade.
+	 * Stop any visible indication on the original channel so we can
+	 * transfer it to the clonechan taking the original's place.
+	 */
+	visible_indication = original->visible_indication;
+	ast_channel_unlock(original);
+	ast_indicate(original, -1);
+
+	/*
+	 * Release any hold on the transferee channel before going any
+	 * further with the masquerade.
 	 */
 	if (xfer_colp && xfer_colp->transferee_held) {
 		ast_indicate(clonechan, AST_CONTROL_UNHOLD);
 	}
 
-	/* 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);
+	/* Start the masquerade channel contents rearangement. */
+	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);
-
-	/*
-	 * Stop any visible indiction on the original channel so we can
-	 * transfer it to the clonechan taking the original's place.
-	 */
-	visible_indication = original->visible_indication;
-	ast_indicate(original, -1);
 
 	chans[0] = clonechan;
 	chans[1] = original;
@@ -6769,8 +6753,12 @@
 		"OriginalState: %s\r\n",
 		clonechan->name, ast_state2str(clonechan->_state), original->name, ast_state2str(original->_state));
 
-	/* Having remembered the original read/write formats, we turn off any translation on either
-	   one */
+	/*
+	 * Remember the original read/write formats.  We turn off any
+	 * translation on either one
+	 */
+	ast_format_copy(&rformat, &original->readformat);
+	ast_format_copy(&wformat, &original->writeformat);
 	free_translation(clonechan);
 	free_translation(original);
 
@@ -6795,14 +6783,14 @@
 	original->tech = clonechan->tech;
 	clonechan->tech = t;
 
+	t_pvt = original->tech_pvt;
+	original->tech_pvt = clonechan->tech_pvt;
+	clonechan->tech_pvt = t_pvt;
+
 	/* Swap the cdrs */
 	cdr = original->cdr;
 	original->cdr = clonechan->cdr;
 	clonechan->cdr = cdr;
-
-	t_pvt = original->tech_pvt;
-	original->tech_pvt = clonechan->tech_pvt;
-	clonechan->tech_pvt = t_pvt;
 
 	/* Swap the alertpipes */
 	for (i = 0; i < 2; i++) {
@@ -6811,7 +6799,7 @@
 		clonechan->alertpipe[i] = x;
 	}
 
-	/* 
+	/*
 	 * Swap the readq's.  The end result should be this:
 	 *
 	 *  1) All frames should be on the new (original) channel.
@@ -6824,8 +6812,8 @@
 	 */
 	{
 		AST_LIST_HEAD_NOLOCK(, ast_frame) tmp_readq;
-		AST_LIST_HEAD_SET_NOLOCK(&tmp_readq, NULL);
-
+
+		AST_LIST_HEAD_INIT_NOLOCK(&tmp_readq);
 		AST_LIST_APPEND_LIST(&tmp_readq, &original->readq, frame_list);
 		AST_LIST_APPEND_LIST(&original->readq, &clonechan->readq, frame_list);
 
@@ -6859,23 +6847,6 @@
 	origstate = original->_state;
 	original->_state = clonechan->_state;
 	clonechan->_state = origstate;
-
-	if (clonechan->tech->fixup && clonechan->tech->fixup(original, clonechan)) {
-		ast_log(LOG_WARNING, "Fixup failed on channel %s, strange things may happen.\n", clonechan->name);
-	}
-
-	/* Start by disconnecting the original's physical side */
-	if (clonechan->tech->hangup && clonechan->tech->hangup(clonechan)) {
-		ast_log(LOG_WARNING, "Hangup failed!  Strange things may happen!\n");
-		res = -1;
-		goto done;
-	}
-
-	/*
-	 * We just hung up the physical side of the channel.  Set the
-	 * new zombie to use the kill channel driver for safety.
-	 */
-	clonechan->tech = &ast_kill_tech;
 
 	/* Mangle the name of the clone channel */
 	snprintf(zombn, sizeof(zombn), "%s<ZOMBIE>", orig); /* quick, hide the brains! */
@@ -6977,37 +6948,108 @@
 	ast_debug(1, "Putting channel %s in %s/%s formats\n", original->name,
 		ast_getformatname(&wformat), ast_getformatname(&rformat));
 
-	/* Okay.  Last thing is to let the channel driver know about all this mess, so he
-	   can fix up everything as best as possible */
-	if (original->tech->fixup) {
-		if (original->tech->fixup(clonechan, original)) {
-			ast_log(LOG_WARNING, "Channel for type '%s' could not fixup channel %s\n",
-				original->tech->type, original->name);
-			res = -1;
-			goto done;
-		}
-	} else
-		ast_log(LOG_WARNING, "Channel type '%s' does not have a fixup routine (for %s)!  Bad things may happen.\n",
+	/* Fixup the original clonechan's physical side */
+	if (original->tech->fixup && original->tech->fixup(clonechan, original)) {
+		ast_log(LOG_WARNING, "Channel type '%s' could not fixup channel %s, strange things may happen. (clonechan)\n",
 			original->tech->type, original->name);
-
-	/* 
-	 * If an indication is currently playing, maintain it on the channel 
-	 * that is taking the place of original 
+	}
+
+	/* Fixup the original original's physical side */
+	if (clonechan->tech->fixup && clonechan->tech->fixup(original, clonechan)) {
+		ast_log(LOG_WARNING, "Channel type '%s' could not fixup channel %s, strange things may happen. (original)\n",
+			clonechan->tech->type, clonechan->name);
+	}
+
+	/*
+	 * Now, at this point, the "clone" channel is totally F'd up.
+	 * We mark it as a zombie so nothing tries to touch it.  If it's
+	 * already been marked as a zombie, then we must free it (since
+	 * it already is considered invalid).
 	 *
-	 * This is needed because the masquerade is swapping out in the internals
-	 * of this channel, and the new channel private data needs to be made
-	 * aware of the current visible indication (RINGING, CONGESTION, etc.)
+	 * This must be done before we unlock clonechan to prevent
+	 * setting up another masquerade on the clonechan.
+	 */
+	if (ast_test_flag(clonechan, AST_FLAG_ZOMBIE)) {
+		clone_was_zombie = 1;
+	} else {
+		ast_set_flag(clonechan, AST_FLAG_ZOMBIE);
+		ast_queue_frame(clonechan, &ast_null_frame);
+	}
+
+	/* clear the masquerade channels */
+	original->masq = NULL;
+	clonechan->masqr = NULL;
+
+	/*
+	 * When we unlock original here, it can be immediately setup to
+	 * masquerade again or hungup.  The new masquerade or hangup
+	 * will not actually happen until we release the channels
+	 * container lock.
+	 */
+	ast_channel_unlock(original);
+
+	/* Disconnect the original original's physical side */
+	if (clonechan->tech->hangup && clonechan->tech->hangup(clonechan)) {
+		ast_log(LOG_WARNING, "Hangup failed!  Strange things may happen!\n");
+	} else {
+		/*
+		 * We just hung up the original original's physical side of the
+		 * channel.  Set the new zombie to use the kill channel driver
+		 * for safety.
+		 */
+		clonechan->tech = &ast_kill_tech;
+	}
+
+	ast_channel_unlock(clonechan);
+
+	/*
+	 * If an indication is currently playing, maintain it on the
+	 * channel that is taking the place of original.
+	 *
+	 * This is needed because the masquerade is swapping out the
+	 * internals of the channel, and the new channel private data
+	 * needs to be made aware of the current visible indication
+	 * (RINGING, CONGESTION, etc.)
 	 */
 	if (visible_indication) {
 		ast_indicate(original, visible_indication);
 	}
 
-	/* Now, at this point, the "clone" channel is totally F'd up.  We mark it as
-	   a zombie so nothing tries to touch it.  If it's already been marked as a
-	   zombie, then free it now (since it already is considered invalid). */
-	if (ast_test_flag(clonechan, AST_FLAG_ZOMBIE)) {
+	ast_channel_lock(original);
+
+	/* Signal any blocker */
+	if (ast_test_flag(original, AST_FLAG_BLOCKING)) {
+		pthread_kill(original->blocker, SIGURG);
+	}
+
+	ast_debug(1, "Done Masquerading %s (%d)\n", original->name, original->_state);
+
+	if ((bridged = ast_bridged_channel(original))) {
+		ast_channel_ref(bridged);
+		ast_channel_unlock(original);
+		ast_indicate(bridged, AST_CONTROL_SRCCHANGE);
+		ast_channel_unref(bridged);
+	} else {
+		ast_channel_unlock(original);
+	}
+	ast_indicate(original, AST_CONTROL_SRCCHANGE);
+
+	if (xfer_colp) {
+		/*
+		 * After the masquerade, the original channel pointer actually
+		 * points to the new transferee channel and the bridged channel
+		 * is still the intended transfer target party.
+		 */
+		masquerade_colp_transfer(original, xfer_colp);
+	}
+
+	if (xfer_ds) {
+		ast_datastore_free(xfer_ds);
+	}
+
+	if (clone_was_zombie) {
+		ast_channel_lock(clonechan);
 		ast_debug(1, "Destroying channel clone '%s'\n", clonechan->name);
-		ast_channel_unlock(clonechan);
 		ast_manager_event(clonechan, EVENT_FLAG_CALL, "Hangup",
 			"Channel: %s\r\n"
 			"Uniqueid: %s\r\n"
@@ -7018,52 +7060,25 @@
 			clonechan->hangupcause,
 			ast_cause2str(clonechan->hangupcause)
 			);
-		clonechan = ast_channel_release(clonechan);
+		ast_channel_unlock(clonechan);
+
+		/*
+		 * Drop the system reference to destroy the channel since it is
+		 * already unlinked.
+		 */
+		ast_channel_unref(clonechan);
 	} else {
-		ast_debug(1, "Released clone lock on '%s'\n", clonechan->name);
-		ast_set_flag(clonechan, AST_FLAG_ZOMBIE);
-		ast_queue_frame(clonechan, &ast_null_frame);
-	}
-
-	/* Signal any blocker */
-	if (ast_test_flag(original, AST_FLAG_BLOCKING))
-		pthread_kill(original->blocker, SIGURG);
-	ast_debug(1, "Done Masquerading %s (%d)\n", original->name, original->_state);
-
-	if ((bridged = ast_bridged_channel(original))) {
-		ast_channel_lock(bridged);
-		ast_indicate(bridged, AST_CONTROL_SRCCHANGE);
-		ast_channel_unlock(bridged);
-	}
-	ast_indicate(original, AST_CONTROL_SRCCHANGE);
-
-	if (xfer_colp) {
-		/*
-		 * After the masquerade, the original channel pointer actually
-		 * points to the new transferee channel and the bridged channel
-		 * is still the intended transfer target party.
-		 */
-		masquerade_colp_transfer(original, xfer_colp);
-	}
-
-done:
-	if (xfer_ds) {
-		ast_datastore_free(xfer_ds);
-	}
-	/* it is possible for the clone channel to disappear during this */
-	if (clonechan) {
-		ast_channel_unlock(original);
-		ast_channel_unlock(clonechan);
 		ao2_link(channels, clonechan);
-		ao2_link(channels, original);
-	} else {
-		ast_channel_unlock(original);
-		ao2_link(channels, original);
-	}
-
+	}
+
+	ao2_link(channels, original);
 	ao2_unlock(channels);
 
-	return res;
+	/* Release our held safety references. */
+	ast_channel_unref(original);
+	ast_channel_unref(clonechan);
+
+	return 0;
 }
 
 void ast_set_callerid(struct ast_channel *chan, const char *cid_num, const char *cid_name, const char *cid_ani)




More information about the asterisk-commits mailing list