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: https://origsvn.digium.com/svn/asterisk/branches/1.8@324364 65c4cc65-6c06-0410-ace0-fbb531ad65f3

diff --git a/include/asterisk/pbx.h b/include/asterisk/pbx.h
index e80a303f605474a7095b071bc45332654a67129a..f75cbea319193d4913cbea2013bf8179e031a09a 100644 (file)
--- 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 174212e3e3d81469c7a3f6e493a1086b33f94174..c26f532b32ca2447a572ebff8405d2398fbf9940 100644 (file)
--- a/main/pbx.c
+++ b/main/pbx.c
@@ -7818,55 +7818,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;
+               format_t readformat;
+               format_t 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;
+       tmpvars.writeformat = chan->writeformat;
+       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 */
+       tmpchan->readformat = tmpvars.readformat;
+       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;
 }