Merge "autochan/mixmonitor/chanspy: Fix unsafe channel locking and references." into 13

4 files changed, 85 insertions, 34 deletions

diff --git a/apps/app_chanspy.c b/apps/app_chanspy.c
index 19675fb28..608eb6be1 100644
--- 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 3515f4b8c..979bf2d70 100644
--- 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 319c203ab..128377b57 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 @@ 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 c1e8b83f3..cf80d8f9b 100644
--- 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);
 	}
 }