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 19675fb28c0b00c4c7d2b51f0cd62fe6e24e3b09..608eb6be185255ed0efbbcdae8d6dfe82c8f0f23 100644 (file)
--- a/apps/app_chanspy.c
+++ b/apps/app_chanspy.c
@@ -498,10 +498,15 @@ static struct ast_generator spygen = {
 
 static int start_spying(struct ast_autochan *autochan, const char *spychan_name, struct ast_audiohook *audiohook)
 {
+       int res;
+
+       ast_autochan_channel_lock(autochan);
        ast_log(LOG_NOTICE, "Attaching %s to %s\n", spychan_name, ast_channel_name(autochan->chan));
 
        ast_set_flag(audiohook, AST_AUDIOHOOK_TRIGGER_SYNC | 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)
@@ -585,8 +590,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;
        }
@@ -598,12 +609,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)) {
                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;
 
@@ -623,21 +632,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);
@@ -829,7 +842,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;
@@ -966,11 +979,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;
 
@@ -984,13 +998,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;
@@ -1008,11 +1028,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));
@@ -1040,7 +1062,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';
@@ -1062,7 +1086,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++) {
@@ -1127,12 +1153,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 3515f4b8c5f236d59e19a06da586a920a41d05b3..979bf2d7044c0e043842f209f923f8774204e53a 100644 (file)
--- a/apps/app_mixmonitor.c
+++ b/apps/app_mixmonitor.c
@@ -622,6 +622,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;
@@ -679,8 +689,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) */
@@ -729,11 +738,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);
 
@@ -805,11 +814,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 c1e8b83f363f2263ca65130a33f43d0056859a20..cf80d8f9b0de00ae18571773e9bf879798084985 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);
        }
 }