[svn-commits] may: branch may/ooh323_qsig r368512 - in /team/may/ooh323_qsig: ./ main/

SVN commits to the Digium repositories svn-commits at lists.digium.com
Mon Jun 4 17:25:23 CDT 2012


Author: may
Date: Mon Jun  4 17:25:18 2012
New Revision: 368512

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=368512
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
........

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

Merged revisions 368421 from http://svn.asterisk.org/svn/asterisk/trunk

Modified:
    team/may/ooh323_qsig/   (props changed)
    team/may/ooh323_qsig/main/channel.c

Propchange: team/may/ooh323_qsig/
------------------------------------------------------------------------------
Binary property 'branch-10-merged' - no diff available.

Propchange: team/may/ooh323_qsig/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Mon Jun  4 17:25:18 2012
@@ -1,1 +1,1 @@
-/trunk:357542
+/trunk:357542,368421

Propchange: team/may/ooh323_qsig/
------------------------------------------------------------------------------
--- svnmerge-integrated (original)
+++ svnmerge-integrated Mon Jun  4 17:25:18 2012
@@ -1,1 +1,1 @@
-/trunk:1-368372
+/trunk:1-368372,368421

Modified: team/may/ooh323_qsig/main/channel.c
URL: http://svnview.digium.com/svn/asterisk/team/may/ooh323_qsig/main/channel.c?view=diff&rev=368512&r1=368511&r2=368512
==============================================================================
--- team/may/ooh323_qsig/main/channel.c (original)
+++ team/may/ooh323_qsig/main/channel.c Mon Jun  4 17:25:18 2012
@@ -2609,13 +2609,7 @@
 	 */
 	while (ast_channel_masq(chan)) {
 		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);
 	}
 
@@ -2631,6 +2625,7 @@
 		return 0;
 	}
 
+	/* Mark as a zombie so a masquerade cannot be setup on this channel. */
 	if (!(was_zombie = ast_test_flag(ast_channel_flags(chan), AST_FLAG_ZOMBIE))) {
 		ast_set_flag(ast_channel_flags(chan), AST_FLAG_ZOMBIE);
 	}
@@ -3008,10 +3003,8 @@
 
 	/* Perform any pending masquerades */
 	for (x = 0; x < n; x++) {
-		if (ast_channel_masq(c[x]) && ast_do_masquerade(c[x])) {
-			ast_log(LOG_WARNING, "Masquerade failed\n");
-			*ms = -1;
-			return NULL;
+		while (ast_channel_masq(c[x])) {
+			ast_do_masquerade(c[x]);
 		}
 
 		ast_channel_lock(c[x]);
@@ -3153,10 +3146,8 @@
 
 
 	/* See if this channel needs to be masqueraded */
-	if (ast_channel_masq(chan) && ast_do_masquerade(chan)) {
-		ast_log(LOG_WARNING, "Failed to perform masquerade on %s\n", ast_channel_name(chan));
-		*ms = -1;
-		return NULL;
+	while (ast_channel_masq(chan)) {
+		ast_do_masquerade(chan);
 	}
 
 	ast_channel_lock(chan);
@@ -3240,10 +3231,8 @@
 	struct ast_channel *winner = NULL;
 
 	for (i = 0; i < n; i++) {
-		if (ast_channel_masq(c[i]) && ast_do_masquerade(c[i])) {
-			ast_log(LOG_WARNING, "Masquerade failed\n");
-			*ms = -1;
-			return NULL;
+		while (ast_channel_masq(c[i])) {
+			ast_do_masquerade(c[i]);
 		}
 
 		ast_channel_lock(c[i]);
@@ -3636,11 +3625,8 @@
 	 */
 
 	if (ast_channel_masq(chan)) {
-		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 */
@@ -4727,12 +4713,9 @@
 		goto done;
 
 	/* Handle any pending masquerades */
-	if (ast_channel_masq(chan)) {
+	while (ast_channel_masq(chan)) {
 		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 (ast_channel_masqr(chan)) {
@@ -6542,9 +6525,9 @@
 int ast_do_masquerade(struct ast_channel *original)
 {
 	int x;
-	int res=0;
 	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;
@@ -6567,60 +6550,64 @@
 	char masqn[AST_CHANNEL_NAME];
 	char zombn[AST_CHANNEL_NAME];
 
-	ast_format_copy(&rformat, ast_channel_readformat(original));
-	ast_format_copy(&wformat, ast_channel_writeformat(original));
-
 	/* 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.
+	/*
+	 * 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.
+	 * 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 = ast_channel_masq(original)) && ast_channel_trylock(clonechan)) {
+	clonechan = ast_channel_masq(original);
+	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);
@@ -6632,30 +6619,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 = ast_channel_visible_indication(original);
+	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 */
-	ast_channel_masq_set(original, NULL);
-	ast_channel_masqr_set(clonechan, 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",
 		ast_channel_name(clonechan), ast_channel_state(clonechan), ast_channel_name(original), ast_channel_state(original));
-
-	/*
-	 * Stop any visible indiction on the original channel so we can
-	 * transfer it to the clonechan taking the original's place.
-	 */
-	visible_indication = ast_channel_visible_indication(original);
-	ast_indicate(original, -1);
 
 	chans[0] = clonechan;
 	chans[1] = original;
@@ -6666,8 +6649,12 @@
 		"OriginalState: %s\r\n",
 		ast_channel_name(clonechan), ast_state2str(ast_channel_state(clonechan)), ast_channel_name(original), ast_state2str(ast_channel_state(original)));
 
-	/* 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, ast_channel_readformat(original));
+	ast_format_copy(&wformat, ast_channel_writeformat(original));
 	free_translation(clonechan);
 	free_translation(original);
 
@@ -6692,14 +6679,14 @@
 	ast_channel_tech_set(original, ast_channel_tech(clonechan));
 	ast_channel_tech_set(clonechan, t);
 
+	t_pvt = ast_channel_tech_pvt(original);
+	ast_channel_tech_pvt_set(original, ast_channel_tech_pvt(clonechan));
+	ast_channel_tech_pvt_set(clonechan, t_pvt);
+
 	/* Swap the cdrs */
 	cdr = ast_channel_cdr(original);
 	ast_channel_cdr_set(original, ast_channel_cdr(clonechan));
 	ast_channel_cdr_set(clonechan, cdr);
-
-	t_pvt = ast_channel_tech_pvt(original);
-	ast_channel_tech_pvt_set(original, ast_channel_tech_pvt(clonechan));
-	ast_channel_tech_pvt_set(clonechan, t_pvt);
 
 	/* Swap the alertpipes */
 	ast_channel_internal_alertpipe_swap(original, clonechan);
@@ -6716,9 +6703,9 @@
 	 *     old (clone) channel.
 	 */
 	{
-		AST_LIST_HEAD_NOLOCK(,ast_frame) tmp_readq;
-		AST_LIST_HEAD_SET_NOLOCK(&tmp_readq, NULL);
-
+		AST_LIST_HEAD_NOLOCK(, ast_frame) tmp_readq;
+
+		AST_LIST_HEAD_INIT_NOLOCK(&tmp_readq);
 		AST_LIST_APPEND_LIST(&tmp_readq, ast_channel_readq(original), frame_list);
 		AST_LIST_APPEND_LIST(ast_channel_readq(original), ast_channel_readq(clonechan), frame_list);
 
@@ -6748,23 +6735,6 @@
 	origstate = ast_channel_state(original);
 	ast_channel_state_set(original, ast_channel_state(clonechan));
 	ast_channel_state_set(clonechan, origstate);
-
-	if (ast_channel_tech(clonechan)->fixup && ast_channel_tech(clonechan)->fixup(original, clonechan)) {
-		ast_log(LOG_WARNING, "Fixup failed on channel %s, strange things may happen.\n", ast_channel_name(clonechan));
-	}
-
-	/* Start by disconnecting the original's physical side */
-	if (ast_channel_tech(clonechan)->hangup && ast_channel_tech(clonechan)->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.
-	 */
-	ast_channel_tech_set(clonechan, &ast_kill_tech);
 
 	/* Mangle the name of the clone channel */
 	snprintf(zombn, sizeof(zombn), "%s<ZOMBIE>", orig); /* quick, hide the brains! */
@@ -6866,37 +6836,108 @@
 	ast_debug(1, "Putting channel %s in %s/%s formats\n", ast_channel_name(original),
 		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 (ast_channel_tech(original)->fixup) {
-		if (ast_channel_tech(original)->fixup(clonechan, original)) {
-			ast_log(LOG_WARNING, "Channel for type '%s' could not fixup channel %s\n",
-				ast_channel_tech(original)->type, ast_channel_name(original));
-			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 (ast_channel_tech(original)->fixup && ast_channel_tech(original)->fixup(clonechan, original)) {
+		ast_log(LOG_WARNING, "Channel type '%s' could not fixup channel %s, strange things may happen. (clonechan)\n",
 			ast_channel_tech(original)->type, ast_channel_name(original));
+	}
+
+	/* Fixup the original original's physical side */
+	if (ast_channel_tech(clonechan)->fixup && ast_channel_tech(clonechan)->fixup(original, clonechan)) {
+		ast_log(LOG_WARNING, "Channel type '%s' could not fixup channel %s, strange things may happen. (original)\n",
+			ast_channel_tech(clonechan)->type, ast_channel_name(clonechan));
+	}
 
 	/*
-	 * If an indication is currently playing, maintain it on the channel
-	 * that is taking the place of original
+	 * 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(ast_channel_flags(clonechan), AST_FLAG_ZOMBIE)) {
+		clone_was_zombie = 1;
+	} else {
+		ast_set_flag(ast_channel_flags(clonechan), AST_FLAG_ZOMBIE);
+		ast_queue_frame(clonechan, &ast_null_frame);
+	}
+
+	/* clear the masquerade channels */
+	ast_channel_masq_set(original, NULL);
+	ast_channel_masqr_set(clonechan, 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 (ast_channel_tech(clonechan)->hangup && ast_channel_tech(clonechan)->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.
+		 */
+		ast_channel_tech_set(clonechan, &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(ast_channel_flags(clonechan), AST_FLAG_ZOMBIE)) {
+	ast_channel_lock(original);
+
+	/* Signal any blocker */
+	if (ast_test_flag(ast_channel_flags(original), AST_FLAG_BLOCKING)) {
+		pthread_kill(ast_channel_blocker(original), SIGURG);
+	}
+
+	ast_debug(1, "Done Masquerading %s (%d)\n", ast_channel_name(original), ast_channel_state(original));
+
+	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", ast_channel_name(clonechan));
-		ast_channel_unlock(clonechan);
 		ast_manager_event(clonechan, EVENT_FLAG_CALL, "Hangup",
 			"Channel: %s\r\n"
 			"Uniqueid: %s\r\n"
@@ -6907,52 +6948,25 @@
 			ast_channel_hangupcause(clonechan),
 			ast_cause2str(ast_channel_hangupcause(clonechan))
 			);
-		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", ast_channel_name(clonechan));
-		ast_set_flag(ast_channel_flags(clonechan), AST_FLAG_ZOMBIE);
-		ast_queue_frame(clonechan, &ast_null_frame);
-	}
-
-	/* Signal any blocker */
-	if (ast_test_flag(ast_channel_flags(original), AST_FLAG_BLOCKING))
-		pthread_kill(ast_channel_blocker(original), SIGURG);
-	ast_debug(1, "Done Masquerading %s (%d)\n", ast_channel_name(original), ast_channel_state(original));
-
-	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 svn-commits mailing list