bridge: Fix softmix bridge deadlock.

* Fix deadlock in
bridge_softmix.c:softmix_bridge_stream_topology_changed() between
bridge_channel and channel locks.

* The new bridge technology topology change callbacks must be called with
the bridge locked.  The callback references the bridge channel list, the
bridge technology could change, and the bridge stream mapping is updated.

ASTERISK-27212

Change-Id: Ide4360ab853607e738ad471721af3f561ddd83be

bridges/bridge_softmix.c

@@ -1690,7 +1690,8 @@ static void map_source_to_destinations(const char *source_stream_name, const cha
 	}
 }
 
-/*\brief stream_topology_changed callback
+/*!
+ * \brief stream_topology_changed callback
  *
  * For most video modes, nothing beyond the ordinary is required.
  * For the SFU case, though, we need to completely remap the streams
@@ -1728,34 +1729,52 @@ static void softmix_bridge_stream_topology_changed(struct ast_bridge *bridge, st
 
 	/* Second traversal: Map specific video channels from their source to their destinations.
 	 *
-	 * This is similar to what is done in ast_stream_topology_map(), except that
-	 * video channels are handled differently. Each video source has it's own
-	 * unique index on the bridge. this way, a particular channel's source video
-	 * can be distributed to the appropriate destination streams on the other
-	 * channels
+	 * This is similar to what is done in ast_stream_topology_map(),
+	 * except that video channels are handled differently.  Each video
+	 * source has it's own unique index on the bridge.  This way, a
+	 * particular channel's source video can be distributed to the
+	 * appropriate destination streams on the other channels.
 	 */
 	AST_LIST_TRAVERSE(&bridge->channels, participant, entry) {
 		int i;
 		struct ast_stream_topology *topology;
 
+		ast_bridge_channel_lock(participant);
 		ast_channel_lock(participant->chan);
 
 		topology = ast_channel_get_stream_topology(participant->chan);
+		if (topology) {
+			/*
+			 * Sigh.  We have to clone to avoid deadlock in
+			 * map_source_to_destinations() because topology
+			 * is not an ao2 object.
+			 */
+			topology = ast_stream_topology_clone(topology);
+		}
+		if (!topology) {
+			/* Oh, my, we are in trouble. */
+			ast_channel_unlock(participant->chan);
+			ast_bridge_channel_unlock(participant);
+			continue;
+		}
 
 		for (i = 0; i < ast_stream_topology_get_count(topology); ++i) {
 			struct ast_stream *stream = ast_stream_topology_get_stream(topology, i);
-			ast_bridge_channel_lock(participant);
+
 			if (is_video_source(stream)) {
 				AST_VECTOR_APPEND(&media_types, AST_MEDIA_TYPE_VIDEO);
 				AST_VECTOR_REPLACE(&participant->stream_map.to_bridge, i, AST_VECTOR_SIZE(&media_types) - 1);
 				AST_VECTOR_REPLACE(&participant->stream_map.to_channel, AST_VECTOR_SIZE(&media_types) - 1, -1);
-				/* Unlock the participant to prevent potential deadlock
-				 * in map_source_to_destinations
+				/*
+				 * Unlock the channel and participant to prevent
+				 * potential deadlock in map_source_to_destinations().
 				 */
+				ast_channel_unlock(participant->chan);
 				ast_bridge_channel_unlock(participant);
 				map_source_to_destinations(ast_stream_get_name(stream), ast_channel_name(participant->chan),
 					AST_VECTOR_SIZE(&media_types) - 1, &bridge->channels);
 				ast_bridge_channel_lock(participant);
+				ast_channel_lock(participant->chan);
 			} else if (is_video_dest(stream, NULL, NULL)) {
 				/* We expect to never read media from video destination channels, but just
 				 * in case, we should set their to_bridge value to -1.
@@ -1777,10 +1796,12 @@ static void softmix_bridge_stream_topology_changed(struct ast_bridge *bridge, st
 				AST_VECTOR_REPLACE(&participant->stream_map.to_bridge, i, index);
 				AST_VECTOR_REPLACE(&participant->stream_map.to_channel, index, i);
 			}
-			ast_bridge_channel_unlock(participant);
 		}
 
+		ast_stream_topology_free(topology);
+
 		ast_channel_unlock(participant->chan);
+		ast_bridge_channel_unlock(participant);
 	}
 }
 

include/asterisk/bridge_channel.h

@@ -716,19 +716,24 @@ void ast_bridge_channel_feature_digit(struct ast_bridge_channel *bridge_channel,
  * \brief Maps a channel's stream topology to and from the bridge
  * \since 15.0.0
  *
- * When a channel joins a bridge or its associated stream topology is updated, each stream
- * in the topology needs to be mapped according to its media type to the bridge. Calling
- * this method creates a mapping of each stream on the channel indexed to the bridge's
- * supported media types and vice versa (i.e. bridge's media types indexed to channel
- * streams).
- *
- * The first channel to join the bridge creates the initial order for the bridge's media
- * types (e.g. a one to one mapping is made). Subsequently added channels are mapped to
- * that order adding more media types if/when the newly added channel has more streams
- * and/or media types specified by the bridge.
+ * \details
+ * When a channel joins a bridge or its associated stream topology is
+ * updated, each stream in the topology needs to be mapped according
+ * to its media type to the bridge.  Calling this method creates a
+ * mapping of each stream on the channel indexed to the bridge's
+ * supported media types and vice versa (i.e. bridge's media types
+ * indexed to channel streams).
+ *
+ * The first channel to join the bridge creates the initial order for
+ * the bridge's media types (e.g. a one to one mapping is made).
+ * Subsequently added channels are mapped to that order adding more
+ * media types if/when the newly added channel has more streams and/or
+ * media types specified by the bridge.
  *
  * \param bridge_channel Channel to map
  *
+ * \note The bridge_channel's bridge must be locked prior to calling this function.
+ *
  * \return Nothing
  */
 void ast_bridge_channel_stream_map(struct ast_bridge_channel *bridge_channel);

include/asterisk/bridge_technology.h

@@ -164,20 +164,30 @@ struct ast_bridge_technology {
 	/*!
 	 * \brief Callback for when a request has been made to change a stream topology on a channel
 	 *
-	 * This is called when a bridge receives a request to change the topology on the channel. A bridge
-	 * technology should define a handler for this callback if it needs to update internals or intercept
-	 * the request and not pass it on to other channels. This can be done by returning a nonzero value.
+	 * \details
+	 * This is called when a bridge receives a request to change the
+	 * topology on the channel.  A bridge technology should define a
+	 * handler for this callback if it needs to update internals or
+	 * intercept the request and not pass it on to other channels.
+	 * This can be done by returning a nonzero value.
 	 *
-	 * \retval 0 Frame accepted by the bridge.
-	 * \retval -1 Frame rejected.
+	 * \retval 0 Frame can pass to the bridge technology.
+	 * \retval non-zero Frame intercepted by the bridge technology.
+	 *
+	 * \note On entry, bridge is already locked.
 	 */
 	int (*stream_topology_request_change)(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel);
 	/*!
 	 * \brief Callback for when a stream topology changes on the channel
 	 *
-	 * This is called when a bridge receives an indication that a topology has been changed on a channel
-	 * and the new topology has been mapped to the bridge. A bridge technology should define a handler for
-	 * this callback if it needs to update internals due to a channel's topology changing.
+	 * \details
+	 * This is called when a bridge receives an indication that a
+	 * topology has been changed on a channel and the new topology has
+	 * been mapped to the bridge.  A bridge technology should define a
+	 * handler for this callback if it needs to update internals due
+	 * to a channel's topology changing.
+	 *
+	 * \note On entry, bridge is already locked.
 	 */
 	void (*stream_topology_changed)(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel);
 	/*! TRUE if the bridge technology is currently suspended. */

main/bridge_channel.c

@@ -2434,6 +2434,7 @@ static const char *controls[] = {
 static void bridge_handle_trip(struct ast_bridge_channel *bridge_channel)
 {
 	struct ast_frame *frame;
+	int blocked;
 
 	if (!ast_strlen_zero(ast_channel_call_forward(bridge_channel->chan))) {
 		/* TODO If early bridging is ever used by anything other than ARI,
@@ -2485,10 +2486,16 @@ static void bridge_handle_trip(struct ast_bridge_channel *bridge_channel)
 			ast_channel_publish_dial(NULL, bridge_channel->chan, NULL, controls[frame->subclass.integer]);
 			break;
 		case AST_CONTROL_STREAM_TOPOLOGY_REQUEST_CHANGE:
-			if (bridge_channel->bridge->technology->stream_topology_request_change &&
-			    bridge_channel->bridge->technology->stream_topology_request_change(
-				    bridge_channel->bridge, bridge_channel)) {
-				/* Topology change was denied so drop frame */
+			ast_bridge_channel_lock_bridge(bridge_channel);
+			blocked = bridge_channel->bridge->technology->stream_topology_request_change
+				&& bridge_channel->bridge->technology->stream_topology_request_change(
+					bridge_channel->bridge, bridge_channel);
+			ast_bridge_unlock(bridge_channel->bridge);
+			if (blocked) {
+				/*
+				 * Topology change was intercepted by the bridge technology
+				 * so drop frame.
+				 */
 				bridge_frame_free(frame);
 				return;
 			}
@@ -2498,12 +2505,14 @@ static void bridge_handle_trip(struct ast_bridge_channel *bridge_channel)
 			 * If a stream topology has changed then the bridge_channel's
 			 * media mapping needs to be updated.
 			 */
+			ast_bridge_channel_lock_bridge(bridge_channel);
 			if (bridge_channel->bridge->technology->stream_topology_changed) {
 				bridge_channel->bridge->technology->stream_topology_changed(
 					bridge_channel->bridge, bridge_channel);
 			} else {
 				ast_bridge_channel_stream_map(bridge_channel);
 			}
+			ast_bridge_unlock(bridge_channel->bridge);
 			break;
 		default:
 			break;
@@ -2990,9 +2999,11 @@ struct ast_bridge_channel *bridge_channel_internal_alloc(struct ast_bridge *brid
 
 void ast_bridge_channel_stream_map(struct ast_bridge_channel *bridge_channel)
 {
+	ast_bridge_channel_lock(bridge_channel);
 	ast_channel_lock(bridge_channel->chan);
 	ast_stream_topology_map(ast_channel_get_stream_topology(bridge_channel->chan),
 		&bridge_channel->bridge->media_types, &bridge_channel->stream_map.to_bridge,
 		&bridge_channel->stream_map.to_channel);
 	ast_channel_unlock(bridge_channel->chan);
+	ast_bridge_channel_unlock(bridge_channel);
 }