<p>Joshua Colp <strong>merged</strong> this change.</p><p><a href="https://gerrit.asterisk.org/6280">View Change</a></p><div style="white-space:pre-wrap">Approvals:
Joshua Colp: Looks good to me, but someone else must approve
George Joseph: Looks good to me, approved
Jenkins2: Approved for Submit
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">bridge: Fix softmix bridge deadlock.<br><br>* Fix deadlock in<br>bridge_softmix.c:softmix_bridge_stream_topology_changed() between<br>bridge_channel and channel locks.<br><br>* The new bridge technology topology change callbacks must be called with<br>the bridge locked. The callback references the bridge channel list, the<br>bridge technology could change, and the bridge stream mapping is updated.<br><br>ASTERISK-27212<br><br>Change-Id: Ide4360ab853607e738ad471721af3f561ddd83be<br>---<br>M bridges/bridge_softmix.c<br>M include/asterisk/bridge_channel.h<br>M include/asterisk/bridge_technology.h<br>M main/bridge_channel.c<br>4 files changed, 78 insertions(+), 31 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">diff --git a/bridges/bridge_softmix.c b/bridges/bridge_softmix.c<br>index b359948..bae0b8c 100644<br>--- a/bridges/bridge_softmix.c<br>+++ b/bridges/bridge_softmix.c<br>@@ -1690,7 +1690,8 @@<br> }<br> }<br> <br>-/*\brief stream_topology_changed callback<br>+/*!<br>+ * \brief stream_topology_changed callback<br> *<br> * For most video modes, nothing beyond the ordinary is required.<br> * For the SFU case, though, we need to completely remap the streams<br>@@ -1728,34 +1729,52 @@<br> <br> /* Second traversal: Map specific video channels from their source to their destinations.<br> *<br>- * This is similar to what is done in ast_stream_topology_map(), except that<br>- * video channels are handled differently. Each video source has it's own<br>- * unique index on the bridge. this way, a particular channel's source video<br>- * can be distributed to the appropriate destination streams on the other<br>- * channels<br>+ * This is similar to what is done in ast_stream_topology_map(),<br>+ * except that video channels are handled differently. Each video<br>+ * source has it's own unique index on the bridge. This way, a<br>+ * particular channel's source video can be distributed to the<br>+ * appropriate destination streams on the other channels.<br> */<br> AST_LIST_TRAVERSE(&bridge->channels, participant, entry) {<br> int i;<br> struct ast_stream_topology *topology;<br> <br>+ ast_bridge_channel_lock(participant);<br> ast_channel_lock(participant->chan);<br> <br> topology = ast_channel_get_stream_topology(participant->chan);<br>+ if (topology) {<br>+ /*<br>+ * Sigh. We have to clone to avoid deadlock in<br>+ * map_source_to_destinations() because topology<br>+ * is not an ao2 object.<br>+ */<br>+ topology = ast_stream_topology_clone(topology);<br>+ }<br>+ if (!topology) {<br>+ /* Oh, my, we are in trouble. */<br>+ ast_channel_unlock(participant->chan);<br>+ ast_bridge_channel_unlock(participant);<br>+ continue;<br>+ }<br> <br> for (i = 0; i < ast_stream_topology_get_count(topology); ++i) {<br> struct ast_stream *stream = ast_stream_topology_get_stream(topology, i);<br>- ast_bridge_channel_lock(participant);<br>+<br> if (is_video_source(stream)) {<br> AST_VECTOR_APPEND(&media_types, AST_MEDIA_TYPE_VIDEO);<br> AST_VECTOR_REPLACE(&participant->stream_map.to_bridge, i, AST_VECTOR_SIZE(&media_types) - 1);<br> AST_VECTOR_REPLACE(&participant->stream_map.to_channel, AST_VECTOR_SIZE(&media_types) - 1, -1);<br>- /* Unlock the participant to prevent potential deadlock<br>- * in map_source_to_destinations<br>+ /*<br>+ * Unlock the channel and participant to prevent<br>+ * potential deadlock in map_source_to_destinations().<br> */<br>+ ast_channel_unlock(participant->chan);<br> ast_bridge_channel_unlock(participant);<br> map_source_to_destinations(ast_stream_get_name(stream), ast_channel_name(participant->chan),<br> AST_VECTOR_SIZE(&media_types) - 1, &bridge->channels);<br> ast_bridge_channel_lock(participant);<br>+ ast_channel_lock(participant->chan);<br> } else if (is_video_dest(stream, NULL, NULL)) {<br> /* We expect to never read media from video destination channels, but just<br> * in case, we should set their to_bridge value to -1.<br>@@ -1777,10 +1796,12 @@<br> AST_VECTOR_REPLACE(&participant->stream_map.to_bridge, i, index);<br> AST_VECTOR_REPLACE(&participant->stream_map.to_channel, index, i);<br> }<br>- ast_bridge_channel_unlock(participant);<br> }<br> <br>+ ast_stream_topology_free(topology);<br>+<br> ast_channel_unlock(participant->chan);<br>+ ast_bridge_channel_unlock(participant);<br> }<br> }<br> <br>diff --git a/include/asterisk/bridge_channel.h b/include/asterisk/bridge_channel.h<br>index 4d33260..4504d6b 100644<br>--- a/include/asterisk/bridge_channel.h<br>+++ b/include/asterisk/bridge_channel.h<br>@@ -716,19 +716,24 @@<br> * \brief Maps a channel's stream topology to and from the bridge<br> * \since 15.0.0<br> *<br>- * When a channel joins a bridge or its associated stream topology is updated, each stream<br>- * in the topology needs to be mapped according to its media type to the bridge. Calling<br>- * this method creates a mapping of each stream on the channel indexed to the bridge's<br>- * supported media types and vice versa (i.e. bridge's media types indexed to channel<br>- * streams).<br>+ * \details<br>+ * When a channel joins a bridge or its associated stream topology is<br>+ * updated, each stream in the topology needs to be mapped according<br>+ * to its media type to the bridge. Calling this method creates a<br>+ * mapping of each stream on the channel indexed to the bridge's<br>+ * supported media types and vice versa (i.e. bridge's media types<br>+ * indexed to channel streams).<br> *<br>- * The first channel to join the bridge creates the initial order for the bridge's media<br>- * types (e.g. a one to one mapping is made). Subsequently added channels are mapped to<br>- * that order adding more media types if/when the newly added channel has more streams<br>- * and/or media types specified by the bridge.<br>+ * The first channel to join the bridge creates the initial order for<br>+ * the bridge's media types (e.g. a one to one mapping is made).<br>+ * Subsequently added channels are mapped to that order adding more<br>+ * media types if/when the newly added channel has more streams and/or<br>+ * media types specified by the bridge.<br> *<br> * \param bridge_channel Channel to map<br> *<br>+ * \note The bridge_channel's bridge must be locked prior to calling this function.<br>+ *<br> * \return Nothing<br> */<br> void ast_bridge_channel_stream_map(struct ast_bridge_channel *bridge_channel);<br>diff --git a/include/asterisk/bridge_technology.h b/include/asterisk/bridge_technology.h<br>index 8cebe93..def7b19 100644<br>--- a/include/asterisk/bridge_technology.h<br>+++ b/include/asterisk/bridge_technology.h<br>@@ -164,20 +164,30 @@<br> /*!<br> * \brief Callback for when a request has been made to change a stream topology on a channel<br> *<br>- * This is called when a bridge receives a request to change the topology on the channel. A bridge<br>- * technology should define a handler for this callback if it needs to update internals or intercept<br>- * the request and not pass it on to other channels. This can be done by returning a nonzero value.<br>+ * \details<br>+ * This is called when a bridge receives a request to change the<br>+ * topology on the channel. A bridge technology should define a<br>+ * handler for this callback if it needs to update internals or<br>+ * intercept the request and not pass it on to other channels.<br>+ * This can be done by returning a nonzero value.<br> *<br>- * \retval 0 Frame accepted by the bridge.<br>- * \retval -1 Frame rejected.<br>+ * \retval 0 Frame can pass to the bridge technology.<br>+ * \retval non-zero Frame intercepted by the bridge technology.<br>+ *<br>+ * \note On entry, bridge is already locked.<br> */<br> int (*stream_topology_request_change)(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel);<br> /*!<br> * \brief Callback for when a stream topology changes on the channel<br> *<br>- * This is called when a bridge receives an indication that a topology has been changed on a channel<br>- * and the new topology has been mapped to the bridge. A bridge technology should define a handler for<br>- * this callback if it needs to update internals due to a channel's topology changing.<br>+ * \details<br>+ * This is called when a bridge receives an indication that a<br>+ * topology has been changed on a channel and the new topology has<br>+ * been mapped to the bridge. A bridge technology should define a<br>+ * handler for this callback if it needs to update internals due<br>+ * to a channel's topology changing.<br>+ *<br>+ * \note On entry, bridge is already locked.<br> */<br> void (*stream_topology_changed)(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel);<br> /*! TRUE if the bridge technology is currently suspended. */<br>diff --git a/main/bridge_channel.c b/main/bridge_channel.c<br>index 1427fd0..c465ec1 100644<br>--- a/main/bridge_channel.c<br>+++ b/main/bridge_channel.c<br>@@ -2434,6 +2434,7 @@<br> static void bridge_handle_trip(struct ast_bridge_channel *bridge_channel)<br> {<br> struct ast_frame *frame;<br>+ int blocked;<br> <br> if (!ast_strlen_zero(ast_channel_call_forward(bridge_channel->chan))) {<br> /* TODO If early bridging is ever used by anything other than ARI,<br>@@ -2485,10 +2486,16 @@<br> ast_channel_publish_dial(NULL, bridge_channel->chan, NULL, controls[frame->subclass.integer]);<br> break;<br> case AST_CONTROL_STREAM_TOPOLOGY_REQUEST_CHANGE:<br>- if (bridge_channel->bridge->technology->stream_topology_request_change &&<br>- bridge_channel->bridge->technology->stream_topology_request_change(<br>- bridge_channel->bridge, bridge_channel)) {<br>- /* Topology change was denied so drop frame */<br>+ ast_bridge_channel_lock_bridge(bridge_channel);<br>+ blocked = bridge_channel->bridge->technology->stream_topology_request_change<br>+ && bridge_channel->bridge->technology->stream_topology_request_change(<br>+ bridge_channel->bridge, bridge_channel);<br>+ ast_bridge_unlock(bridge_channel->bridge);<br>+ if (blocked) {<br>+ /*<br>+ * Topology change was intercepted by the bridge technology<br>+ * so drop frame.<br>+ */<br> bridge_frame_free(frame);<br> return;<br> }<br>@@ -2498,12 +2505,14 @@<br> * If a stream topology has changed then the bridge_channel's<br> * media mapping needs to be updated.<br> */<br>+ ast_bridge_channel_lock_bridge(bridge_channel);<br> if (bridge_channel->bridge->technology->stream_topology_changed) {<br> bridge_channel->bridge->technology->stream_topology_changed(<br> bridge_channel->bridge, bridge_channel);<br> } else {<br> ast_bridge_channel_stream_map(bridge_channel);<br> }<br>+ ast_bridge_unlock(bridge_channel->bridge);<br> break;<br> default:<br> break;<br>@@ -2990,9 +2999,11 @@<br> <br> void ast_bridge_channel_stream_map(struct ast_bridge_channel *bridge_channel)<br> {<br>+ ast_bridge_channel_lock(bridge_channel);<br> ast_channel_lock(bridge_channel->chan);<br> ast_stream_topology_map(ast_channel_get_stream_topology(bridge_channel->chan),<br> &bridge_channel->bridge->media_types, &bridge_channel->stream_map.to_bridge,<br> &bridge_channel->stream_map.to_channel);<br> ast_channel_unlock(bridge_channel->chan);<br>+ ast_bridge_channel_unlock(bridge_channel);<br> }<br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/6280">change 6280</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/6280"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15.0 </div>
<div style="display:none"> Gerrit-MessageType: merged </div>
<div style="display:none"> Gerrit-Change-Id: Ide4360ab853607e738ad471721af3f561ddd83be </div>
<div style="display:none"> Gerrit-Change-Number: 6280 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>