[Asterisk-code-review] autochan/mixmonitor/chanspy: Fix unsafe channel locking and ... (asterisk[master])
Anonymous Coward
asteriskteam at digium.com
Tue Mar 21 21:51:50 CDT 2017
Anonymous Coward #1000019 has submitted this change and it was merged. ( https://gerrit.asterisk.org/5221 )
Change subject: autochan/mixmonitor/chanspy: Fix unsafe channel locking and references.
......................................................................
autochan/mixmonitor/chanspy: Fix unsafe channel locking and references.
Dereferencing struct ast_autochan.chan without first calling
ast_autochan_channel_lock() is unsafe because the pointer could change at
any time due to a masquerade. Unfortunately, ast_autochan_channel_lock()
itself uses struct ast_autochan.chan unsafely and can result in a deadlock
if the original channel happens to get destroyed after a masquerade in
addition to the pointer getting changed.
The problem is more likely to happen with v11 and earlier because
masquerades are used to optimize out local channels on those versions.
However, it could still happen on newer versions if the channel is
executing a dialplan application when the channel is transferred or
redirected. In this situation a masquerade still must be used.
* Added a lock to struct ast_autochan to safely be able to use
ast_autochan.chan while trying to get the channel lock in
ast_autochan_channel_lock(). The locking order is the channel lock then
the autochan lock. Locking in the other direction requires deadlock
avoidance.
* Fix unsafe ast_autochan.chan usages in app_mixmonitor.c.
* Fix unsafe ast_autochan.chan usages in app_chanspy.c.
* app_chanspy.c: Removed unused autochan parameter from next_channel().
ASTERISK-26867
Change-Id: Id29dd22bc0f369b44e23ca423d2f3657187cc592
---
M apps/app_chanspy.c
M apps/app_mixmonitor.c
M include/asterisk/autochan.h
M main/autochan.c
4 files changed, 88 insertions(+), 36 deletions(-)
Approvals:
George Joseph: 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 50d8d63..4814f4c 100644
--- a/apps/app_chanspy.c
+++ b/apps/app_chanspy.c
@@ -506,18 +506,24 @@
static int start_spying(struct ast_autochan *autochan, const char *spychan_name, struct ast_audiohook *audiohook, struct ast_flags *flags)
{
+ int res;
+
+ ast_autochan_channel_lock(autochan);
ast_log(LOG_NOTICE, "Attaching %s to %s\n", spychan_name, ast_channel_name(autochan->chan));
- if(ast_test_flag(flags, OPTION_READONLY)) {
+
+ if (ast_test_flag(flags, OPTION_READONLY)) {
ast_set_flag(audiohook, AST_AUDIOHOOK_MUTE_WRITE);
} else {
ast_set_flag(audiohook, AST_AUDIOHOOK_TRIGGER_SYNC);
}
- if(ast_test_flag(flags, OPTION_LONG_QUEUE)) {
+ if (ast_test_flag(flags, OPTION_LONG_QUEUE)) {
ast_debug(9, "Using a long queue to store audio frames in spy audiohook\n");
} else {
ast_set_flag(audiohook, AST_AUDIOHOOK_SMALL_QUEUE);
}
- return ast_audiohook_attach(autochan->chan, audiohook);
+ res = ast_audiohook_attach(autochan->chan, audiohook);
+ ast_autochan_channel_unlock(autochan);
+ return res;
}
static void change_spy_mode(const char digit, struct ast_flags *flags)
@@ -601,8 +607,14 @@
{
int retval = 0;
struct ast_autochan *internal_bridge_autochan;
- RAII_VAR(struct ast_channel *, bridged, ast_channel_bridge_peer(spyee_autochan->chan), ast_channel_cleanup);
+ struct ast_channel *spyee_chan;
+ RAII_VAR(struct ast_channel *, bridged, NULL, ast_channel_cleanup);
+ ast_autochan_channel_lock(spyee_autochan);
+ spyee_chan = ast_channel_ref(spyee_autochan->chan);
+ ast_autochan_channel_unlock(spyee_autochan);
+ bridged = ast_channel_bridge_peer(spyee_chan);
+ ast_channel_unref(spyee_chan);
if (!bridged) {
return -1;
}
@@ -614,12 +626,10 @@
return -1;
}
- ast_autochan_channel_lock(internal_bridge_autochan);
if (start_spying(internal_bridge_autochan, spyer_name, bridge_whisper_audiohook, flags)) {
ast_log(LOG_WARNING, "Unable to attach barge audiohook on spyee '%s'. Barge mode disabled.\n", name);
retval = -1;
}
- ast_autochan_channel_unlock(internal_bridge_autochan);
*spyee_bridge_autochan = internal_bridge_autochan;
@@ -639,21 +649,25 @@
struct ast_autochan *spyee_bridge_autochan = NULL;
const char *spyer_name;
- if (ast_check_hangup(chan) || ast_check_hangup(spyee_autochan->chan) ||
- ast_test_flag(ast_channel_flags(spyee_autochan->chan), AST_FLAG_ZOMBIE)) {
+ ast_channel_lock(chan);
+ if (ast_check_hangup(chan)) {
+ ast_channel_unlock(chan);
return 0;
}
-
- ast_channel_lock(chan);
spyer_name = ast_strdupa(ast_channel_name(chan));
ast_channel_unlock(chan);
ast_autochan_channel_lock(spyee_autochan);
+ if (ast_check_hangup(spyee_autochan->chan)
+ || ast_test_flag(ast_channel_flags(spyee_autochan->chan), AST_FLAG_ZOMBIE)) {
+ ast_autochan_channel_unlock(spyee_autochan);
+ return 0;
+ }
name = ast_strdupa(ast_channel_name(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);
+ ast_autochan_channel_unlock(spyee_autochan);
memset(&csth, 0, sizeof(csth));
ast_copy_flags(&csth.flags, flags, AST_FLAGS_ALL);
@@ -845,7 +859,7 @@
}
static struct ast_autochan *next_channel(struct ast_channel_iterator *iter,
- struct ast_autochan *autochan, struct ast_channel *chan)
+ struct ast_channel *chan)
{
struct ast_channel *next;
struct ast_autochan *autochan_store;
@@ -982,11 +996,12 @@
waitms = 100;
num_spyed_upon = 0;
- for (autochan = next_channel(iter, autochan, chan);
- autochan;
- prev = autochan->chan, ast_autochan_destroy(autochan),
- autochan = next_autochan ? next_autochan :
- next_channel(iter, autochan, chan), next_autochan = NULL) {
+ for (autochan = next_channel(iter, chan);
+ autochan;
+ prev = autochan->chan,
+ ast_autochan_destroy(autochan),
+ autochan = next_autochan ?: next_channel(iter, chan),
+ next_autochan = NULL) {
int igrp = !mygroup;
int ienf = !myenforced;
@@ -1000,13 +1015,19 @@
break;
}
- if (ast_test_flag(flags, OPTION_BRIDGED) && !ast_channel_is_bridged(autochan->chan)) {
+ ast_autochan_channel_lock(autochan);
+ if (ast_test_flag(flags, OPTION_BRIDGED)
+ && !ast_channel_is_bridged(autochan->chan)) {
+ ast_autochan_channel_unlock(autochan);
continue;
}
- if (ast_check_hangup(autochan->chan) || ast_test_flag(ast_channel_flags(autochan->chan), AST_FLAG_SPYING)) {
+ if (ast_check_hangup(autochan->chan)
+ || ast_test_flag(ast_channel_flags(autochan->chan), AST_FLAG_SPYING)) {
+ ast_autochan_channel_unlock(autochan);
continue;
}
+ ast_autochan_channel_unlock(autochan);
if (mygroup) {
int num_groups = 0;
@@ -1024,11 +1045,13 @@
/* Before dahdi scan was part of chanspy, it would use the "GROUP" variable
* rather than "SPYGROUP", this check is done to preserve expected behavior */
+ ast_autochan_channel_lock(autochan);
if (ast_test_flag(flags, OPTION_DAHDI_SCAN)) {
group = pbx_builtin_getvar_helper(autochan->chan, "GROUP");
} else {
group = pbx_builtin_getvar_helper(autochan->chan, "SPYGROUP");
}
+ ast_autochan_channel_unlock(autochan);
if (!ast_strlen_zero(group)) {
ast_copy_string(dup_group, group, sizeof(dup_group));
@@ -1056,7 +1079,9 @@
snprintf(buffer, sizeof(buffer) - 1, ":%s:", myenforced);
+ ast_autochan_channel_lock(autochan);
ast_copy_string(ext + 1, ast_channel_name(autochan->chan), sizeof(ext) - 1);
+ ast_autochan_channel_unlock(autochan);
if ((end = strchr(ext, '-'))) {
*end++ = ':';
*end = '\0';
@@ -1078,7 +1103,9 @@
char *ptr, *s;
strcpy(peer_name, "spy-");
+ ast_autochan_channel_lock(autochan);
strncat(peer_name, ast_channel_name(autochan->chan), AST_NAME_STRLEN - 4 - 1);
+ ast_autochan_channel_unlock(autochan);
if ((ptr = strchr(peer_name, '/'))) {
*ptr++ = '\0';
for (s = peer_name; s < ptr; s++) {
@@ -1143,12 +1170,14 @@
next = ast_channel_unref(next);
} else {
/* stay on this channel, if it is still valid */
+ ast_autochan_channel_lock(autochan);
if (!ast_check_hangup(autochan->chan)) {
next_autochan = ast_autochan_setup(autochan->chan);
} else {
/* the channel is gone */
next_autochan = NULL;
}
+ ast_autochan_channel_unlock(autochan);
}
} else if (res == 0 && ast_test_flag(flags, OPTION_EXITONHANGUP)) {
ast_autochan_destroy(autochan);
diff --git a/apps/app_mixmonitor.c b/apps/app_mixmonitor.c
index a5a00cc..3258b30 100644
--- a/apps/app_mixmonitor.c
+++ b/apps/app_mixmonitor.c
@@ -617,6 +617,16 @@
}
}
+static int mixmonitor_autochan_is_bridged(struct ast_autochan *autochan)
+{
+ int is_bridged;
+
+ ast_autochan_channel_lock(autochan);
+ is_bridged = ast_channel_is_bridged(autochan->chan);
+ ast_autochan_channel_unlock(autochan);
+ return is_bridged;
+}
+
static void *mixmonitor_thread(void *obj)
{
struct mixmonitor *mixmonitor = obj;
@@ -674,8 +684,7 @@
ast_audiohook_unlock(&mixmonitor->audiohook);
if (!ast_test_flag(mixmonitor, MUXFLAG_BRIDGED)
- || (mixmonitor->autochan->chan
- && ast_channel_is_bridged(mixmonitor->autochan->chan))) {
+ || mixmonitor_autochan_is_bridged(mixmonitor->autochan)) {
ast_mutex_lock(&mixmonitor->mixmonitor_ds->lock);
/* Write out the frame(s) */
@@ -724,11 +733,11 @@
ast_audiohook_unlock(&mixmonitor->audiohook);
- ast_autochan_channel_lock(mixmonitor->autochan);
if (ast_test_flag(mixmonitor, MUXFLAG_BEEP_STOP)) {
+ ast_autochan_channel_lock(mixmonitor->autochan);
ast_stream_and_wait(mixmonitor->autochan->chan, "beep", "");
+ ast_autochan_channel_unlock(mixmonitor->autochan);
}
- ast_autochan_channel_unlock(mixmonitor->autochan);
ast_autochan_destroy(mixmonitor->autochan);
@@ -800,11 +809,11 @@
return -1;
}
- ast_autochan_channel_lock(mixmonitor->autochan);
if (ast_test_flag(mixmonitor, MUXFLAG_BEEP_START)) {
+ ast_autochan_channel_lock(mixmonitor->autochan);
ast_stream_and_wait(mixmonitor->autochan->chan, "beep", "");
+ ast_autochan_channel_unlock(mixmonitor->autochan);
}
- 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 319c203..128377b 100644
--- a/include/asterisk/autochan.h
+++ b/include/asterisk/autochan.h
@@ -32,6 +32,7 @@
struct ast_autochan {
struct ast_channel *chan;
AST_LIST_ENTRY(ast_autochan) list;
+ ast_mutex_t lock;
};
/*!
@@ -61,19 +62,24 @@
* 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
+ * While chan is locked, the autochan structure is guaranteed to keep the
* same channel.
*/
+/*!
+ * \brief Lock the autochan's channel lock.
+ *
+ * \note We must do deadlock avoidance because the channel lock is
+ * superior to the autochan lock in locking order.
+ */
#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_mutex_lock(&(autochan)->lock); \
+ while (ast_channel_trylock((autochan)->chan)) { \
+ DEADLOCK_AVOIDANCE(&(autochan)->lock); \
} \
- ast_channel_unlock(autochan_chan); \
- } while (1)
+ ast_mutex_unlock(&(autochan)->lock); \
+ } while (0)
#define ast_autochan_channel_unlock(autochan) \
ast_channel_unlock(autochan->chan)
diff --git a/main/autochan.c b/main/autochan.c
index c7e5c00..68aeaf8 100644
--- a/main/autochan.c
+++ b/main/autochan.c
@@ -46,14 +46,17 @@
if (!(autochan = ast_calloc(1, sizeof(*autochan)))) {
return NULL;
}
+ ast_mutex_init(&autochan->lock);
autochan->chan = ast_channel_ref(chan);
- ast_channel_lock(autochan->chan); /* autochan is still private, no need for ast_autochan_channel_lock() */
+ ast_debug(1, "Created autochan %p to hold channel %s (%p)\n",
+ autochan, ast_channel_name(chan), chan);
+
+ /* autochan is still private, no need for ast_autochan_channel_lock() */
+ ast_channel_lock(autochan->chan);
AST_LIST_INSERT_TAIL(ast_channel_autochans(autochan->chan), autochan, list);
ast_channel_unlock(autochan->chan);
-
- ast_debug(1, "Created autochan %p to hold channel %s (%p)\n", autochan, ast_channel_name(chan), chan);
return autochan;
}
@@ -75,6 +78,8 @@
autochan->chan = ast_channel_unref(autochan->chan);
+ ast_mutex_destroy(&autochan->lock);
+
ast_free(autochan);
}
@@ -84,13 +89,16 @@
AST_LIST_APPEND_LIST(ast_channel_autochans(new_chan), ast_channel_autochans(old_chan), list);
+ /* Deadlock avoidance is not needed since the channels are already locked. */
AST_LIST_TRAVERSE(ast_channel_autochans(new_chan), autochan, list) {
+ ast_mutex_lock(&autochan->lock);
if (autochan->chan == old_chan) {
- autochan->chan = ast_channel_unref(old_chan);
autochan->chan = ast_channel_ref(new_chan);
+ ast_channel_unref(old_chan);
ast_debug(1, "Autochan %p used to hold channel %s (%p) but now holds channel %s (%p)\n",
autochan, ast_channel_name(old_chan), old_chan, ast_channel_name(new_chan), new_chan);
}
+ ast_mutex_unlock(&autochan->lock);
}
}
--
To view, visit https://gerrit.asterisk.org/5221
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Id29dd22bc0f369b44e23ca423d2f3657187cc592
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
More information about the asterisk-code-review
mailing list