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

2 files changed, 63 insertions, 25 deletions

diff --git a/apps/app_chanspy.c b/apps/app_chanspy.c
index 50d8d6321..4814f4ce5 100644
--- a/apps/app_chanspy.c
+++ b/apps/app_chanspy.c
@@ -506,18 +506,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)
@@ -601,8 +607,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;
 	}
@@ -614,12 +626,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;
 
@@ -639,21 +649,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);
@@ -845,7 +859,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;
@@ -982,11 +996,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;
 
@@ -1000,13 +1015,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;
@@ -1024,11 +1045,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));
@@ -1056,7 +1079,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';
@@ -1078,7 +1103,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++) {
@@ -1143,12 +1170,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 a5a00cc4e..3258b301f 100644
--- a/apps/app_mixmonitor.c
+++ b/apps/app_mixmonitor.c
@@ -617,6 +617,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;
@@ -674,8 +684,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) */
@@ -724,11 +733,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);
 
@@ -800,11 +809,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;