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

diff --git a/apps/app_chanspy.c b/apps/app_chanspy.c
index df2deae3f5407bb64c31c7a65845e17898c007fe..9ef7477e6f53a981b5068ad1f69bb9cfd7c74723 100644 (file)
--- a/apps/app_chanspy.c
+++ b/apps/app_chanspy.c
@@ -508,18 +508,24 @@ static struct ast_generator spygen = {
 
 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)
@@ -603,8 +609,14 @@ static int attach_barge(struct ast_autochan *spyee_autochan,
 {
        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;
        }
@@ -616,12 +628,10 @@ static int attach_barge(struct ast_autochan *spyee_autochan,
                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;
 
@@ -641,21 +651,25 @@ static int channel_spy(struct ast_channel *chan, struct ast_autochan *spyee_auto
        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);
@@ -847,7 +861,7 @@ static int channel_spy(struct ast_channel *chan, struct ast_autochan *spyee_auto
 }
 
 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;
@@ -984,11 +998,12 @@ static int common_exec(struct ast_channel *chan, struct ast_flags *flags,
                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;
 
@@ -1002,13 +1017,19 @@ static int common_exec(struct ast_channel *chan, struct ast_flags *flags,
                                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;
@@ -1026,11 +1047,13 @@ static int common_exec(struct ast_channel *chan, struct ast_flags *flags,
 
                                /* 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));
@@ -1058,7 +1081,9 @@ static int common_exec(struct ast_channel *chan, struct ast_flags *flags,
 
                                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';
@@ -1080,7 +1105,9 @@ static int common_exec(struct ast_channel *chan, struct ast_flags *flags,
                                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++) {
@@ -1145,12 +1172,14 @@ static int common_exec(struct ast_channel *chan, struct ast_flags *flags,
                                        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 7d7a0cbf914b40f3cebc40671fcf9012042718ff..8de4995afaf8548a4de66a19e8236156da1ad2ba 100644 (file)
--- a/apps/app_mixmonitor.c
+++ b/apps/app_mixmonitor.c
@@ -619,6 +619,16 @@ static void mixmonitor_save_prep(struct mixmonitor *mixmonitor, char *filename,
        }
 }
 
+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;
@@ -676,8 +686,7 @@ static void *mixmonitor_thread(void *obj)
                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) */
@@ -726,11 +735,11 @@ static void *mixmonitor_thread(void *obj)
 
        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);
 
@@ -802,11 +811,11 @@ static int setup_mixmonitor_ds(struct mixmonitor *mixmonitor, struct ast_channel
                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 319c203ab1557d74da6c3ded4de388cfb33dcbf7..128377b57bf296f94c29154648fabb134e847220 100644 (file)
--- 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 @@ struct ast_autochan {
  * 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 38d778438cbd7de81955e55cf26ffe0c691eaa9c..535788fecb988b087320bd8e4239691f83f0a3c6 100644 (file)
--- a/main/autochan.c
+++ b/main/autochan.c
@@ -48,15 +48,18 @@ struct ast_autochan *ast_autochan_setup(struct ast_channel *chan)
        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;
 }
 
@@ -77,6 +80,8 @@ void ast_autochan_destroy(struct ast_autochan *autochan)
 
        autochan->chan = ast_channel_unref(autochan->chan);
 
+       ast_mutex_destroy(&autochan->lock);
+
        ast_free(autochan);
 }
 
@@ -86,13 +91,16 @@ void ast_autochan_new_channel(struct ast_channel *old_chan, struct ast_channel *
 
        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);
        }
 }