Merged revisions 324364 via svnmerge from

https://origsvn.digium.com/svn/asterisk/branches/1.8

........
  r324364 | dvossel | 2011-06-21 15:11:52 -0500 (Tue, 21 Jun 2011) | 10 lines
  
  Fixes locking inversion issue in ast_async_goto()
  
  During this function we can not hold the "chan" lock while
  doing the masquerade, the explicit goto on the tmp chan, or
  the channel alloc.  Instead we need to get the channel lock,
  store off information about the channel that we need, and
  then let the channel lock go for the remainder of the function.
  
  Review: https://reviewboard.asterisk.org/r/1275/
........


git-svn-id: http://svn.digium.com/svn/asterisk/trunk@324365 f38db490-d61c-443f-a65b-d21fe96a405b

2 files changed, 69 insertions, 40 deletions

diff --git a/include/asterisk/pbx.h b/include/asterisk/pbx.h
index 429e60537..1489ee2d6 100644
--- a/include/asterisk/pbx.h
+++ b/include/asterisk/pbx.h
@@ -877,6 +877,8 @@ int ast_context_unlockmacro(const char *macrocontext);
 /*!
  * \brief Set the channel to next execute the specified dialplan location.
  * \see ast_async_parseable_goto, ast_async_goto_if_exists
+ *
+ * \note Do _NOT_ hold any channel locks when calling this function.
  */
 int ast_async_goto(struct ast_channel *chan, const char *context, const char *exten, int priority);
 
diff --git a/main/pbx.c b/main/pbx.c
index 60de17f89..821fdbfad 100644
--- a/main/pbx.c
+++ b/main/pbx.c
@@ -7949,55 +7949,82 @@ int ast_explicit_goto(struct ast_channel *chan, const char *context, const char
 int ast_async_goto(struct ast_channel *chan, const char *context, const char *exten, int priority)
 {
 	int res = 0;
+	struct ast_channel *tmpchan;
+	struct {
+		char *accountcode;
+		char *exten;
+		char *context;
+		char *linkedid;
+		char *name;
+		struct ast_cdr *cdr;
+		int amaflags;
+		int state;
+		struct ast_format readformat;
+		struct ast_format writeformat;
+	} tmpvars = { 0, };
 
 	ast_channel_lock(chan);
-
 	if (chan->pbx) { /* This channel is currently in the PBX */
 		ast_explicit_goto(chan, context, exten, priority + 1);
 		ast_softhangup_nolock(chan, AST_SOFTHANGUP_ASYNCGOTO);
+		ast_channel_unlock(chan);
+		return res;
+	}
+
+	/* In order to do it when the channel doesn't really exist within
+	 * the PBX, we have to make a new channel, masquerade, and start the PBX
+	 * at the new location */
+	tmpvars.accountcode = ast_strdupa(chan->accountcode);
+	tmpvars.exten = ast_strdupa(chan->exten);
+	tmpvars.context = ast_strdupa(chan->context);
+	tmpvars.linkedid = ast_strdupa(chan->linkedid);
+	tmpvars.name = ast_strdupa(chan->name);
+	tmpvars.amaflags = chan->amaflags;
+	tmpvars.state = chan->_state;
+	ast_format_copy(&tmpvars.writeformat, &chan->writeformat);
+	ast_format_copy(&tmpvars.readformat, &chan->readformat);
+	tmpvars.cdr = chan->cdr ? ast_cdr_dup(chan->cdr) : NULL;
+
+	ast_channel_unlock(chan);
+
+	/* Do not hold any channel locks while calling channel_alloc() since the function
+	 * locks the channel container when linking the new channel in. */
+	if (!(tmpchan = ast_channel_alloc(0, tmpvars.state, 0, 0, tmpvars.accountcode, tmpvars.exten, tmpvars.context, tmpvars.linkedid, tmpvars.amaflags, "AsyncGoto/%s", tmpvars.name))) {
+		ast_cdr_discard(tmpvars.cdr);
+		return -1;
+	}
+
+	/* copy the cdr info over */
+	if (tmpvars.cdr) {
+		ast_cdr_discard(tmpchan->cdr);
+		tmpchan->cdr = tmpvars.cdr;
+		tmpvars.cdr = NULL;
+	}
+
+	/* Make formats okay */
+	ast_format_copy(&tmpchan->readformat, &tmpvars.readformat);
+	ast_format_copy(&tmpchan->writeformat, &tmpvars.writeformat);
+
+	/* Setup proper location. Never hold another channel lock while calling this function. */
+	ast_explicit_goto(tmpchan, S_OR(context, tmpvars.context), S_OR(exten, tmpvars.exten), priority);
+
+	/* Masquerade into tmp channel */
+	if (ast_channel_masquerade(tmpchan, chan)) {
+		/* Failed to set up the masquerade.  It's probably chan_local
+		 * in the middle of optimizing itself out.  Sad. :( */
+		ast_hangup(tmpchan);
+		tmpchan = NULL;
+		res = -1;
 	} else {
-		/* In order to do it when the channel doesn't really exist within
-		   the PBX, we have to make a new channel, masquerade, and start the PBX
-		   at the new location */
-		struct ast_channel *tmpchan = ast_channel_alloc(0, chan->_state, 0, 0, chan->accountcode, chan->exten, chan->context, chan->linkedid, chan->amaflags, "AsyncGoto/%s", chan->name);
-		if (!tmpchan) {
+		ast_do_masquerade(tmpchan);
+		/* 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);
+			ast_hangup(tmpchan);
 			res = -1;
-		} else {
-			if (chan->cdr) {
-				ast_cdr_discard(tmpchan->cdr);
-				tmpchan->cdr = ast_cdr_dup(chan->cdr);  /* share the love */
-			}
-			/* Make formats okay */
-			tmpchan->readformat = chan->readformat;
-			tmpchan->writeformat = chan->writeformat;
-			/* Setup proper location */
-			ast_explicit_goto(tmpchan,
-				S_OR(context, chan->context), S_OR(exten, chan->exten), priority);
-
-			/* Masquerade into temp channel */
-			if (ast_channel_masquerade(tmpchan, chan)) {
-				/* Failed to set up the masquerade.  It's probably chan_local
-				 * in the middle of optimizing itself out.  Sad. :( */
-				ast_hangup(tmpchan);
-				tmpchan = NULL;
-				res = -1;
-			} else {
-				/* 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_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);
-					ast_hangup(tmpchan);
-					res = -1;
-				}
-			}
 		}
 	}
-	ast_channel_unlock(chan);
+
 	return res;
 }