[Asterisk-code-review] bridge simple.c: Fix stream topology handling. (asterisk[15])

[Richard Mudgett]

Richard Mudgett has uploaded this change for review. ( https://gerrit.asterisk.org/8308


Change subject: bridge_simple.c: Fix stream topology handling.
......................................................................

bridge_simple.c: Fix stream topology handling.

The handling of stream topologies was not protected by channel locks in
simple_bridge_request_stream_topology_change().

* Fixed topology handling to be protected by channel locks where needed in
simple_bridge_request_stream_topology_change().

ASTERISK-27692

Change-Id: Ica5d78a6c7ecf4f0b95fb16de28d3889b32c4776
---
M bridges/bridge_simple.c
1 file changed, 53 insertions(+), 33 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/08/8308/1

diff --git a/bridges/bridge_simple.c b/bridges/bridge_simple.c
index 7ee1966..be1ec43 100644
--- a/bridges/bridge_simple.c
+++ b/bridges/bridge_simple.c
@@ -114,18 +114,34 @@
 };
 
 static void simple_bridge_request_stream_topology_change(struct ast_channel *chan,
-	struct ast_stream_topology *requested_topology)
+	struct ast_channel *requested_chan)
 {
-	struct ast_stream_topology *existing_topology = ast_channel_get_stream_topology(chan);
+	struct ast_stream_topology *existing_topology;
+	struct ast_stream_topology *requested_topology;
 	struct ast_stream *stream;
 	struct ast_format_cap *audio_formats = NULL;
 	struct ast_stream_topology *new_topology;
 	int i;
 
+	/*
+	 * Sigh.  We have to clone to avoid the request_chan's current topology
+	 * going away on us because we have to give up the lock and topology
+	 * is not an ao2 object.
+	 */
+	requested_topology = ast_channel_get_stream_topology(requested_chan);
+	new_topology = ast_stream_topology_clone(requested_topology);
+	ast_channel_unlock(requested_chan);
+	if (!new_topology) {
+		/* Complete failure.  We'll just have to live with it. */
+		ast_channel_unlock(chan);
+		return;
+	}
+
 	/* We find an existing stream with negotiated audio formats that we can place into
 	 * any audio streams in the new topology to ensure that negotiation succeeds. Some
 	 * endpoints incorrectly terminate the call if SDP negotiation fails.
 	 */
+	existing_topology = ast_channel_get_stream_topology(chan);
 	for (i = 0; i < ast_stream_topology_get_count(existing_topology); ++i) {
 		stream = ast_stream_topology_get_stream(existing_topology, i);
 
@@ -134,63 +150,67 @@
 			continue;
 		}
 
-		audio_formats = ast_stream_get_formats(stream);
+		audio_formats = ao2_bump(ast_stream_get_formats(stream));
 		break;
 	}
 
-	if (!audio_formats) {
-		ast_channel_request_stream_topology_change(chan, requested_topology, &simple_bridge);
-		return;
-	}
+	ast_channel_unlock(chan);
 
-	new_topology = ast_stream_topology_clone(requested_topology);
-	if (!new_topology) {
-		ast_channel_request_stream_topology_change(chan, requested_topology, &simple_bridge);
-		return;
-	}
+	if (audio_formats) {
+		for (i = 0; i < ast_stream_topology_get_count(new_topology); ++i) {
+			stream = ast_stream_topology_get_stream(new_topology, i);
 
-	for (i = 0; i < ast_stream_topology_get_count(new_topology); ++i) {
-		stream = ast_stream_topology_get_stream(new_topology, i);
+			if (ast_stream_get_type(stream) != AST_MEDIA_TYPE_AUDIO ||
+				ast_stream_get_state(stream) == AST_STREAM_STATE_REMOVED) {
+				continue;
+			}
 
-		if (ast_stream_get_type(stream) != AST_MEDIA_TYPE_AUDIO ||
-			ast_stream_get_state(stream) == AST_STREAM_STATE_REMOVED) {
-			continue;
+			ast_format_cap_append_from_cap(ast_stream_get_formats(stream), audio_formats,
+				AST_MEDIA_TYPE_AUDIO);
 		}
-
-		ast_format_cap_append_from_cap(ast_stream_get_formats(stream), audio_formats, AST_MEDIA_TYPE_AUDIO);
+		ao2_ref(audio_formats, -1);
 	}
 
 	ast_channel_request_stream_topology_change(chan, new_topology, &simple_bridge);
-
 	ast_stream_topology_free(new_topology);
 }
 
 static void simple_bridge_stream_topology_changed(struct ast_bridge *bridge,
 		struct ast_bridge_channel *bridge_channel)
 {
-	struct ast_channel *c0 = AST_LIST_FIRST(&bridge->channels)->chan;
-	struct ast_channel *c1 = AST_LIST_LAST(&bridge->channels)->chan;
-	struct ast_stream_topology *t0 = ast_channel_get_stream_topology(c0);
-	struct ast_stream_topology *t1 = ast_channel_get_stream_topology(c1);
+	struct ast_channel *c0;
+	struct ast_channel *c1;
+	struct ast_stream_topology *t0;
+	struct ast_stream_topology *t1;
 
 	if (bridge_channel) {
 		ast_bridge_channel_stream_map(bridge_channel);
+
+		if (ast_channel_get_stream_topology_change_source(bridge_channel->chan)
+			== &simple_bridge) {
+			return;
+		}
 	}
-	/*
-	 * The bridge_channel should only be NULL after both channels join
-	 * the bridge and their topologies are being aligned.
-	 */
-	if (bridge_channel && ast_channel_get_stream_topology_change_source(
-		    bridge_channel->chan) == &simple_bridge) {
+
+	c0 = AST_LIST_FIRST(&bridge->channels)->chan;
+	c1 = AST_LIST_LAST(&bridge->channels)->chan;
+	if (c0 == c1) {
+		/*
+		 * Wait until there is another channel in the bridge
+		 * to align topologies.
+		 */
 		return;
 	}
 
 	/* Align topologies according to size or first channel to join */
+	ast_channel_lock_both(c0, c1);
+	t0 = ast_channel_get_stream_topology(c0);
+	t1 = ast_channel_get_stream_topology(c1);
 	if (ast_stream_topology_get_count(t0) < ast_stream_topology_get_count(t1)) {
-		simple_bridge_request_stream_topology_change(c0, t1);
-	} else {
-		simple_bridge_request_stream_topology_change(c1, t0);
+		SWAP(c0, c1);
 	}
+	simple_bridge_request_stream_topology_change(c1, c0);
+	/* c0 and c1 are unlocked by simple_bridge_request_stream_topology_change() */
 }
 
 static int unload_module(void)

-- 
To view, visit https://gerrit.asterisk.org/8308
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ica5d78a6c7ecf4f0b95fb16de28d3889b32c4776
Gerrit-Change-Number: 8308
Gerrit-PatchSet: 1
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180220/e97d0adc/attachment.html>