[Asterisk-code-review] bridge: Fix softmix bridge deadlock. (asterisk[15])
Richard Mudgett
asteriskteam at digium.com
Tue Aug 22 12:01:11 CDT 2017
Richard Mudgett has uploaded this change for review. ( https://gerrit.asterisk.org/6272
Change subject: bridge: Fix softmix bridge deadlock.
......................................................................
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
---
M bridges/bridge_softmix.c
M include/asterisk/bridge_channel.h
M include/asterisk/bridge_technology.h
M main/bridge_channel.c
4 files changed, 78 insertions(+), 31 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/72/6272/1
diff --git a/bridges/bridge_softmix.c b/bridges/bridge_softmix.c
index b359948..bae0b8c 100644
--- a/bridges/bridge_softmix.c
+++ b/bridges/bridge_softmix.c
@@ -1690,7 +1690,8 @@
}
}
-/*\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 @@
/* 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 @@
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);
}
}
diff --git a/include/asterisk/bridge_channel.h b/include/asterisk/bridge_channel.h
index 4d33260..4504d6b 100644
--- a/include/asterisk/bridge_channel.h
+++ b/include/asterisk/bridge_channel.h
@@ -716,19 +716,24 @@
* \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).
+ * \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.
+ * 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);
diff --git a/include/asterisk/bridge_technology.h b/include/asterisk/bridge_technology.h
index 8cebe93..def7b19 100644
--- a/include/asterisk/bridge_technology.h
+++ b/include/asterisk/bridge_technology.h
@@ -164,20 +164,30 @@
/*!
* \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. */
diff --git a/main/bridge_channel.c b/main/bridge_channel.c
index 1427fd0..c465ec1 100644
--- a/main/bridge_channel.c
+++ b/main/bridge_channel.c
@@ -2434,6 +2434,7 @@
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 @@
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 @@
* 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 @@
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);
}
--
To view, visit https://gerrit.asterisk.org/6272
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ide4360ab853607e738ad471721af3f561ddd83be
Gerrit-Change-Number: 6272
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/20170822/3a9f53b5/attachment-0001.html>
More information about the asterisk-code-review
mailing list