diff options
| author | David Vossel <dvossel@digium.com> | 2009-10-07 22:58:38 +0000 |
|---|---|---|
| committer | David Vossel <dvossel@digium.com> | 2009-10-07 22:58:38 +0000 |
| commit | 9456ab2724901b565c7cdb19973e5785647a27ba (patch) | |
| tree | 31296f94172a5a376dfc1c3c764a970f0b723425 /main/features.c | |
| parent | 49b90d5e61dd158e963cd5d623cbca50e8b4f1c8 (diff) | |
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
Diffstat (limited to 'main/features.c')
| -rw-r--r-- | main/features.c | 13 |
1 files changed, 8 insertions, 5 deletions
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) { |