[asterisk-commits] app chanspy: Fix occasional deadlock with ChanSpy and Local ... (asterisk[master])
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Mon Mar 14 11:18:42 CDT 2016
Anonymous Coward #1000019 has submitted this change and it was merged.
Change subject: app_chanspy: Fix occasional deadlock with ChanSpy and Local channels.
......................................................................
app_chanspy: Fix occasional deadlock with ChanSpy and Local channels.
Channel masquerading had a conflict with autochannel locking.
When locking autochannel->channel, the channel is fetched from the
autochannel and then locked. During the fetch, the autochannel -- which
has no locks itself -- can be modified by someone who owns the channel
lock. That means that the value of autochan->channel cannot be trusted
until you hold the lock.
In practice, this caused problems with Local channels getting
masqueraded away while the ChanSpy attempted to get info from that
channel. The old channel which was about to get removed got locked, but
the new (replaced) channel got unlocked (no-op). Because the replaced
channel was now locked (and would never get unlocked), it couldn't get
removed from the channel list in a timely manner, and would now cause
deadlocks when iterating over the channel list.
This change checks the autochannel after locking the channel for changes
to the autochannel. If the channel had been changed, the lock is
reobtained on the new channel.
In theory it seems possible that after this fix, the lock attempt on the
old (wrong) channel can be on an already destroyed lock, maybe causing
a crash. But that hasn't been observed in the wild and is harder induce
than the current deadlock.
Thanks go to Filip Frank for suggesting a fix similar to this and
especially to IRC user hexanol for pointing out why this deadlock was
possible and testing this fix. And to Richard for catching my rookie
while loop mistake ;)
ASTERISK-25321 #close
Change-Id: I293ae0014e531cd0e675c3f02d1d118a98683def
---
M apps/app_chanspy.c
M apps/app_mixmonitor.c
M include/asterisk/autochan.h
M main/autochan.c
4 files changed, 31 insertions(+), 11 deletions(-)
Approvals:
Richard Mudgett: Looks good to me, but someone else must approve
Anonymous Coward #1000019: Verified
Joshua Colp: Looks good to me, approved
diff --git a/apps/app_chanspy.c b/apps/app_chanspy.c
index 3c7a917..5dbb0b8 100644
--- a/apps/app_chanspy.c
+++ b/apps/app_chanspy.c
@@ -598,12 +598,12 @@
return -1;
}
- ast_channel_lock(internal_bridge_autochan->chan);
+ ast_autochan_channel_lock(internal_bridge_autochan);
if (start_spying(internal_bridge_autochan, spyer_name, bridge_whisper_audiohook)) {
ast_log(LOG_WARNING, "Unable to attach barge audiohook on spyee '%s'. Barge mode disabled.\n", name);
retval = -1;
}
- ast_channel_unlock(internal_bridge_autochan->chan);
+ ast_autochan_channel_unlock(internal_bridge_autochan);
*spyee_bridge_autochan = internal_bridge_autochan;
@@ -632,9 +632,9 @@
spyer_name = ast_strdupa(ast_channel_name(chan));
ast_channel_unlock(chan);
- ast_channel_lock(spyee_autochan->chan);
+ ast_autochan_channel_lock(spyee_autochan);
name = ast_strdupa(ast_channel_name(spyee_autochan->chan));
- ast_channel_unlock(spyee_autochan->chan);
+ ast_autochan_channel_unlock(spyee_autochan);
ast_verb(2, "Spying on channel %s\n", name);
publish_chanspy_message(chan, spyee_autochan->chan, 1);
diff --git a/apps/app_mixmonitor.c b/apps/app_mixmonitor.c
index f5f9b22..7d7a0cb 100644
--- a/apps/app_mixmonitor.c
+++ b/apps/app_mixmonitor.c
@@ -726,11 +726,11 @@
ast_audiohook_unlock(&mixmonitor->audiohook);
- ast_channel_lock(mixmonitor->autochan->chan);
+ ast_autochan_channel_lock(mixmonitor->autochan);
if (ast_test_flag(mixmonitor, MUXFLAG_BEEP_STOP)) {
ast_stream_and_wait(mixmonitor->autochan->chan, "beep", "");
}
- ast_channel_unlock(mixmonitor->autochan->chan);
+ ast_autochan_channel_unlock(mixmonitor->autochan);
ast_autochan_destroy(mixmonitor->autochan);
@@ -802,11 +802,11 @@
return -1;
}
- ast_channel_lock(mixmonitor->autochan->chan);
+ ast_autochan_channel_lock(mixmonitor->autochan);
if (ast_test_flag(mixmonitor, MUXFLAG_BEEP_START)) {
ast_stream_and_wait(mixmonitor->autochan->chan, "beep", "");
}
- ast_channel_unlock(mixmonitor->autochan->chan);
+ ast_autochan_channel_unlock(mixmonitor->autochan);
mixmonitor_ds->samp_rate = 8000;
mixmonitor_ds->audiohook = &mixmonitor->audiohook;
diff --git a/include/asterisk/autochan.h b/include/asterisk/autochan.h
index a0981b7..319c203 100644
--- a/include/asterisk/autochan.h
+++ b/include/asterisk/autochan.h
@@ -56,8 +56,28 @@
* to save off the pointer using ast_channel_ref and to unref the channel when you
* are finished with the pointer. If you do not do this and a masquerade occurs on
* the channel, then it is possible that your saved pointer will become invalid.
+ *
+ * 3. If you want to lock the autochan->chan channel, be sure to use
+ * ast_autochan_channel_lock and ast_autochan_channel_unlock. An attempt to lock
+ * the autochan->chan directly may result in it being changed after you've
+ * retrieved the value of chan, but before you've had a chance to lock it.
+ * First when chan is locked, the autochan structure is guaranteed to keep the
+ * same channel.
*/
+#define ast_autochan_channel_lock(autochan) \
+ do { \
+ struct ast_channel *autochan_chan = autochan->chan; \
+ ast_channel_lock(autochan_chan); \
+ if (autochan->chan == autochan_chan) { \
+ break; \
+ } \
+ ast_channel_unlock(autochan_chan); \
+ } while (1)
+
+#define ast_autochan_channel_unlock(autochan) \
+ ast_channel_unlock(autochan->chan)
+
/*!
* \brief set up a new ast_autochan structure
*
diff --git a/main/autochan.c b/main/autochan.c
index d41a8d8..38d7784 100644
--- a/main/autochan.c
+++ b/main/autochan.c
@@ -51,7 +51,7 @@
autochan->chan = ast_channel_ref(chan);
- ast_channel_lock(autochan->chan);
+ ast_channel_lock(autochan->chan); /* autochan is still private, no need for ast_autochan_channel_lock() */
AST_LIST_INSERT_TAIL(ast_channel_autochans(autochan->chan), autochan, list);
ast_channel_unlock(autochan->chan);
@@ -64,7 +64,7 @@
{
struct ast_autochan *autochan_iter;
- ast_channel_lock(autochan->chan);
+ ast_autochan_channel_lock(autochan);
AST_LIST_TRAVERSE_SAFE_BEGIN(ast_channel_autochans(autochan->chan), autochan_iter, list) {
if (autochan_iter == autochan) {
AST_LIST_REMOVE_CURRENT(list);
@@ -73,7 +73,7 @@
}
}
AST_LIST_TRAVERSE_SAFE_END;
- ast_channel_unlock(autochan->chan);
+ ast_autochan_channel_unlock(autochan);
autochan->chan = ast_channel_unref(autochan->chan);
--
To view, visit https://gerrit.asterisk.org/2376
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I293ae0014e531cd0e675c3f02d1d118a98683def
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Walter Doekes <walter+asterisk at wjd.nu>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
More information about the asterisk-commits
mailing list