From 87908a3f5d75f5644c0334e551010f7dda5c9973 Mon Sep 17 00:00:00 2001 From: David Vossel Date: Tue, 21 Jun 2011 20:11:52 +0000 Subject: [PATCH] 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 --- include/asterisk/pbx.h | 2 + main/pbx.c | 107 ++++++++++++++++++++++++++--------------- 2 files changed, 69 insertions(+), 40 deletions(-) diff --git a/include/asterisk/pbx.h b/include/asterisk/pbx.h index e80a303f60..f75cbea319 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 174212e3e3..c26f532b32 100644 --- 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; } -- 2.47.2