Deadlock in channel masquerade handling

Channels are stored in an ao2_container.  When accessing an item within
an ao2_container the proper locking order is to first lock the container,
and then the items within it.

In ast_do_masquerade both the clone and original channel must be locked
for the entire duration of the function.  The problem with this is that
it attemptes to unlink and link these channels back into the ao2_container
when one of the channel's name changes.  This is invalid locking order as
the process of unlinking and linking will lock the ao2_container while
the channels are locked!!! Now, both the channels in do_masquerade are
unlinked from the ao2_container and then locked for the entire function.
At the end of the function both channels are unlocked and linked back
into the container with their new names as hash values.

This new method of requiring all channels and tech pvts to be unlocked
before ast_do_masquerade() or ast_change_name() required several
changes throughout the code base.

(closes issue #15911)
Reported by: russell
Patches:
      masq_deadlock_trunk.diff uploaded by dvossel (license 671)
Tested by: dvossel, atis

(closes issue #15618)
Reported by: lmsteffan
Patches:
      deadlock_local_attended_transfers_trunk.diff uploaded by dvossel (license 671)
Tested by: lmsteffan, dvossel

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



git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@222761 65c4cc65-6c06-0410-ace0-fbb531ad65f3

3 files changed, 121 insertions, 57 deletions

diff --git a/main/channel.c b/main/channel.c
index 6c0d5b65b..0e150d6b8 100644
--- a/main/channel.c
+++ b/main/channel.c
@@ -2143,8 +2143,11 @@ int ast_hangup(struct ast_channel *chan)
 	ast_autoservice_stop(chan);
 
 	if (chan->masq) {
-		if (ast_do_masquerade(chan))
+		ast_channel_unlock(chan);
+		if (ast_do_masquerade(chan)) {
 			ast_log(LOG_WARNING, "Failed to perform masquerade\n");
+		}
+		ast_channel_lock(chan);
 	}
 
 	if (chan->masq) {
@@ -2514,13 +2517,13 @@ struct ast_channel *ast_waitfor_nandfds(struct ast_channel **c, int n, int *fds,
 	
 	/* Perform any pending masquerades */
 	for (x = 0; x < n; x++) {
-		ast_channel_lock(c[x]);
 		if (c[x]->masq && ast_do_masquerade(c[x])) {
 			ast_log(LOG_WARNING, "Masquerade failed\n");
 			*ms = -1;
-			ast_channel_unlock(c[x]);
 			return NULL;
 		}
+
+		ast_channel_lock(c[x]);
 		if (!ast_tvzero(c[x]->whentohangup)) {
 			if (ast_tvzero(whentohangup))
 				now = ast_tvnow();
@@ -2641,16 +2644,15 @@ static struct ast_channel *ast_waitfor_nandfds_simple(struct ast_channel *chan,
 	struct ast_channel *winner = NULL;
 	struct ast_epoll_data *aed = NULL;
 
-	ast_channel_lock(chan);
 
 	/* 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;
-		ast_channel_unlock(chan);
 		return NULL;
 	}
 
+	ast_channel_lock(chan);
 	/* Figure out their timeout */
 	if (!ast_tvzero(chan->whentohangup)) {
 		if ((diff = ast_tvdiff_ms(chan->whentohangup, ast_tvnow())) < 0) {
@@ -2726,13 +2728,13 @@ static struct ast_channel *ast_waitfor_nandfds_complex(struct ast_channel **c, i
 	struct ast_channel *winner = NULL;
 
 	for (i = 0; i < n; i++) {
-		ast_channel_lock(c[i]);
 		if (c[i]->masq && ast_do_masquerade(c[i])) {
 			ast_log(LOG_WARNING, "Masquerade failed\n");
 			*ms = -1;
-			ast_channel_unlock(c[i]);
 			return NULL;
 		}
+
+		ast_channel_lock(c[i]);
 		if (!ast_tvzero(c[i]->whentohangup)) {
 			if (whentohangup == 0)
 				now = ast_tvnow();
@@ -3064,26 +3066,23 @@ static struct ast_frame *__ast_read(struct ast_channel *chan, int dropaudio)
 	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).
 	 */
-	while(ast_channel_trylock(chan)) {
-		if(count++ > 10) 
-			/*cannot goto done since the channel is not locked*/
-			return &ast_null_frame;
-		usleep(1);
-	}
 
 	if (chan->masq) {
 		if (ast_do_masquerade(chan))
 			ast_log(LOG_WARNING, "Failed to perform masquerade\n");
 		else
 			f =  &ast_null_frame;
-		goto done;
+		return f;
 	}
 
+	/* if here, no masq has happened, lock the channel and proceed */
+	ast_channel_lock(chan);
+
 	/* Stop if we're a zombie or need a soft hangup */
 	if (ast_test_flag(chan, AST_FLAG_ZOMBIE) || ast_check_hangup(chan)) {
 		if (chan->generator)
@@ -3893,9 +3892,13 @@ int ast_write(struct ast_channel *chan, struct ast_frame *fr)
 		goto done;
 
 	/* Handle any pending masquerades */
-	if (chan->masq && ast_do_masquerade(chan)) {
-		ast_log(LOG_WARNING, "Failed to perform masquerade\n");
-		goto done;
+	if (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_channel_lock(chan);
 	}
 	if (chan->masqr) {
 		res = 0;	/* XXX explain, why 0 ? */
@@ -4806,15 +4809,23 @@ retrymasq:
 	return res;
 }
 
-void ast_change_name(struct ast_channel *chan, const char *newname)
+/*! \brief this function simply changes the name of the channel and issues a manager_event
+ *         with out unlinking and linking the channel from the ao2_container.  This should
+ *         only be used when the channel has already been unlinked from the ao2_container.
+ */
+static void __ast_change_name_nolink(struct ast_channel *chan, const char *newname)
 {
-	/* We must re-link, as the hash value will change here. */
-	ao2_unlink(channels, chan);
-
 	manager_event(EVENT_FLAG_CALL, "Rename", "Channel: %s\r\nNewname: %s\r\nUniqueid: %s\r\n", chan->name, newname, chan->uniqueid);
-
 	ast_string_field_set(chan, name, newname);
+}
 
+void ast_change_name(struct ast_channel *chan, const char *newname)
+{
+	/* We must re-link, as the hash value will change here. */
+	ao2_unlink(channels, chan);
+	ast_channel_lock(chan);
+	__ast_change_name_nolink(chan, newname);
+	ast_channel_unlock(chan);
 	ao2_link(channels, chan);
 }
 
@@ -5063,7 +5074,9 @@ static void report_new_callerid(const struct ast_channel *chan)
 /*!
   \brief Masquerade a channel
 
-  \note Assumes channel will be locked when called
+  \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.
 */
 int ast_do_masquerade(struct ast_channel *original)
 {
@@ -5078,7 +5091,7 @@ int ast_do_masquerade(struct ast_channel *original)
 		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;
@@ -5092,8 +5105,52 @@ int ast_do_masquerade(struct ast_channel *original)
 	 * original channel's backend.  While the features are nice, which is the
 	 * reason we're keeping it, it's still awesomely weird. XXX */
 
-	/* We need the clone's lock, too */
-	ast_channel_lock(clonechan);
+	/* 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.
+	 */
+	ao2_lock(channels);
+
+	/* lock the original channel to determine if the masquerade is require or not */
+	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 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. */
+	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 both channels are locked and unlinked from the container, it is safe to unlock it */
+	ao2_unlock(channels);
 
 	ast_debug(4, "Actually Masquerading %s(%d) into the structure of %s(%d)\n",
 		clonechan->name, clonechan->_state, original->name, original->_state);
@@ -5106,10 +5163,6 @@ int ast_do_masquerade(struct ast_channel *original)
 	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));
 	/* Save the new name */
@@ -5118,10 +5171,10 @@ int ast_do_masquerade(struct ast_channel *original)
 	snprintf(masqn, sizeof(masqn), "%s<MASQ>", newn);
 
 	/* Mangle the name of the clone channel */
-	ast_change_name(clonechan, masqn);
+	__ast_change_name_nolink(clonechan, masqn);
 
 	/* Copy the name from the clone channel */
-	ast_change_name(original, newn);
+	__ast_change_name_nolink(original, newn);
 
 	/* share linked id's */
 	ast_channel_set_linkgroup(original, clonechan);
@@ -5195,24 +5248,20 @@ int ast_do_masquerade(struct ast_channel *original)
 	original->_state = clonechan->_state;
 	clonechan->_state = origstate;
 
-	if (clonechan->tech->fixup){
-		res = clonechan->tech->fixup(original, clonechan);
-		if (res)
-			ast_log(LOG_WARNING, "Fixup failed on channel %s, strange things may happen.\n", clonechan->name);
+	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)
-		res = clonechan->tech->hangup(clonechan);
-	if (res) {
+	if (clonechan->tech->hangup && clonechan->tech->hangup(clonechan)) {
 		ast_log(LOG_WARNING, "Hangup failed!  Strange things may happen!\n");
-		ast_channel_unlock(clonechan);
-		return -1;
+		res = -1;
+		goto done;
 	}
 
 	/* Mangle the name of the clone channel */
-	snprintf(zombn, sizeof(zombn), "%s<ZOMBIE>", orig);
-	ast_change_name(clonechan, zombn);
+	snprintf(zombn, sizeof(zombn), "%s<ZOMBIE>", orig); /* quick, hide the brains! */
+	__ast_change_name_nolink(clonechan, zombn);
 
 	/* Update the type. */
 	t_pvt = original->monitor;
@@ -5306,12 +5355,11 @@ int ast_do_masquerade(struct ast_channel *original)
 	/* 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) {
-		res = original->tech->fixup(clonechan, original);
-		if (res) {
+		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);
-			ast_channel_unlock(clonechan);
-			return -1;
+			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",
@@ -5350,13 +5398,24 @@ int ast_do_masquerade(struct ast_channel *original)
 		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);
-		ast_channel_unlock(clonechan);
 	}
 
 	/* 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);
+
+done:
+	/* 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);
+	}
 	return 0;
 }
 
diff --git a/main/features.c b/main/features.c
index 4ec336b02..61072addf 100644
--- a/main/features.c
+++ b/main/features.c
@@ -4378,17 +4378,19 @@ static char *handle_features_reload(struct ast_cli_entry *e, int cmd, struct ast
 static void do_bridge_masquerade(struct ast_channel *chan, struct ast_channel *tmpchan)
 {
 	ast_moh_stop(chan);
-	ast_channel_lock(chan);
+	ast_channel_lock_both(chan, tmpchan);
 	ast_setstate(tmpchan, chan->_state);
 	tmpchan->readformat = chan->readformat;
 	tmpchan->writeformat = chan->writeformat;
 	ast_channel_masquerade(tmpchan, chan);
-	ast_channel_lock(tmpchan);
+	ast_channel_unlock(chan);
+	ast_channel_unlock(tmpchan);
+
+	/* must be done without any channel locks held */
 	ast_do_masquerade(tmpchan);
+
 	/* when returning from bridge, the channel will continue at the next priority */
 	ast_explicit_goto(tmpchan, chan->context, chan->exten, chan->priority + 1);
-	ast_channel_unlock(tmpchan);
-	ast_channel_unlock(chan);
 }
 
 /*!
@@ -5004,10 +5006,11 @@ static int bridge_exec(struct ast_channel *chan, const char *data)
 					"Channel1: %s\r\n"
 					"Channel2: %s\r\n", chan->name, args.dest_chan);
 	}
-	do_bridge_masquerade(current_dest_chan, final_dest_chan);
 
 	ast_channel_unlock(current_dest_chan);
 
+	do_bridge_masquerade(current_dest_chan, final_dest_chan);
+
 	/* now current_dest_chan is a ZOMBIE and with softhangup set to 1 and final_dest_chan is our end point */
 	/* try to make compatible, send error if we fail */
 	if (ast_channel_make_compatible(chan, final_dest_chan) < 0) {
diff --git a/main/pbx.c b/main/pbx.c
index 0cbaff39f..dd2016ed2 100644
--- a/main/pbx.c
+++ b/main/pbx.c
@@ -7662,10 +7662,12 @@ int ast_async_goto(struct ast_channel *chan, const char *context, const char *ex
 				tmpchan = NULL;
 				res = -1;
 			} else {
-				/* Grab the locks and get going */
-				ast_channel_lock(tmpchan);
+				/* it may appear odd to unlock chan here since the masquerade is on
+				 * tmpchan, but no channel locks should be held when doing a masquerade
+				 * since a masquerade requires a lock on the channels ao2 container. */
+				ast_channel_unlock(chan);
 				ast_do_masquerade(tmpchan);
-				ast_channel_unlock(tmpchan);
+				ast_channel_lock(chan);
 				/* Start the PBX going on our stolen channel */
 				if (ast_pbx_start(tmpchan)) {
 					ast_log(LOG_WARNING, "Unable to start PBX on %s\n", tmpchan->name);