<p>George Joseph has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.asterisk.org/5855">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">bridge_native_rtp: Keep references to rtp instances on hook data<br><br>There have been reports of deadlocks caused by an attempt to send<br>a frame to a channel's rtp instance after the channel has left the<br>native bridge and been destroyed. This patch effectively causes the<br>frame hook to keep a reference to both the audio and video rtp<br>instances until after the hook has been detached.<br><br>ASTERISK-26978 #close<br>Reported-by: Ross Beer<br><br>Change-Id: I9e1ac49fa4af68d64826ccccd152593cf8cdb21a<br>---<br>M bridges/bridge_native_rtp.c<br>1 file changed, 103 insertions(+), 7 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/55/5855/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">diff --git a/bridges/bridge_native_rtp.c b/bridges/bridge_native_rtp.c<br>index 832e397..2d2a156 100644<br>--- a/bridges/bridge_native_rtp.c<br>+++ b/bridges/bridge_native_rtp.c<br>@@ -44,14 +44,19 @@<br> #include "asterisk/bridge.h"<br> #include "asterisk/bridge_technology.h"<br> #include "asterisk/frame.h"<br>+#include "asterisk/linkedlists.h"<br> #include "asterisk/rtp_engine.h"<br> <br> /*! \brief Internal structure which contains information about bridged RTP channels */<br>-struct native_rtp_bridge_data {<br>+struct native_rtp_bridge_channel_data {<br> /*! \brief Framehook used to intercept certain control frames */<br> int id;<br> /*! \brief Set when this framehook has been detached */<br> unsigned int detached;<br>+ /*! \brief This channel's audio rtp instance */<br>+ struct ast_rtp_instance *audio_instance;<br>+ /*! \brief This channel's video rtp instance */<br>+ struct ast_rtp_instance *video_instance;<br> };<br> <br> /*! \brief Internal helper function which gets all RTP information (glue and instances) relating to the given channels */<br>@@ -120,6 +125,26 @@<br> <br> /*!<br> * \internal<br>+ * \brief For debugging bridge lifecycle only<br>+ */<br>+static int native_rtp_bridge_create(struct ast_bridge *bridge)<br>+{<br>+ ast_debug(2, "Bridge '%s' created\n", bridge->uniqueid);<br>+<br>+ return 0;<br>+}<br>+<br>+/*!<br>+ * \internal<br>+ * \brief For debugging bridge lifecycle only<br>+ */<br>+static void native_rtp_bridge_destroy(struct ast_bridge *bridge)<br>+{<br>+ ast_debug(2, "Bridge '%s' destroyed\n", bridge->uniqueid);<br>+}<br>+<br>+/*!<br>+ * \internal<br> * \brief Start native RTP bridging of two channels<br> *<br> * \param bridge The bridge that had native RTP bridging happening on it<br>@@ -143,8 +168,14 @@<br> RAII_VAR(struct ast_format_cap *, cap1, ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT), ao2_cleanup);<br> <br> if (bc0 == bc1) {<br>+ ast_debug(2, "Bridge '%s' doesn't start when both channels are '%s'\n", bridge->uniqueid,<br>+ ast_channel_name(bc0->chan));<br> return;<br> }<br>+<br>+ ast_debug(2, "Bridge '%s' starting with channels '%s' and '%s' and target: '%s'\n", bridge->uniqueid,<br>+ ast_channel_name(bc0->chan), ast_channel_name(bc1->chan),<br>+ target ? ast_channel_name(target) : "none");<br> <br> ast_channel_lock_both(bc0->chan, bc1->chan);<br> if (!bc0->suspended && !bc1->suspended) {<br>@@ -161,6 +192,8 @@<br> }<br> ast_rtp_instance_set_bridged(instance0, instance1);<br> ast_rtp_instance_set_bridged(instance1, instance0);<br>+ ast_debug(2, "Locally RTP bridged '%s' and '%s' in stack\n",<br>+ ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));<br> ast_verb(4, "Locally RTP bridged '%s' and '%s' in stack\n",<br> ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));<br> break;<br>@@ -177,6 +210,8 @@<br> if (!target) {<br> glue0->update_peer(bc0->chan, instance1, vinstance1, tinstance1, cap1, 0);<br> glue1->update_peer(bc1->chan, instance0, vinstance0, tinstance0, cap0, 0);<br>+ ast_debug(2, "Remotely bridged '%s' and '%s' - media will flow directly between them\n",<br>+ ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));<br> ast_verb(4, "Remotely bridged '%s' and '%s' - media will flow directly between them\n",<br> ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));<br> } else {<br>@@ -194,6 +229,10 @@<br> }<br> break;<br> case AST_RTP_GLUE_RESULT_FORBID:<br>+ ast_log(LOG_ERROR, "FORBID returned while attempting to START bridge between '%s' and '%s' "<br>+ "with target '%s'\n",<br>+ ast_channel_name(bc0->chan), ast_channel_name(bc1->chan),<br>+ target ? ast_channel_name(target) : "none");<br> break;<br> }<br> <br>@@ -213,8 +252,14 @@<br> RAII_VAR(struct ast_rtp_instance *, vinstance1, NULL, ao2_cleanup);<br> <br> if (bc0 == bc1) {<br>+ ast_debug(2, "Bridge '%s' doesn't stop when both channels are '%s'\n", bridge->uniqueid,<br>+ ast_channel_name(bc0->chan));<br> return;<br> }<br>+<br>+ ast_debug(2, "Bridge '%s' stopping with channels '%s' and '%s'. Target: '%s'\n", bridge->uniqueid,<br>+ ast_channel_name(bc0->chan), ast_channel_name(bc1->chan),<br>+ target ? ast_channel_name(target) : "none");<br> <br> ast_channel_lock_both(bc0->chan, bc1->chan);<br> native_type = native_rtp_bridge_get(bc0->chan, bc1->chan, &glue0, &glue1, &instance0, &instance1, &vinstance0, &vinstance1);<br>@@ -246,6 +291,10 @@<br> }<br> break;<br> case AST_RTP_GLUE_RESULT_FORBID:<br>+ ast_log(LOG_ERROR, "FORBID returned while attempting to STOP bridge between '%s' and '%s' "<br>+ "with target '%s'\n",<br>+ ast_channel_name(bc0->chan), ast_channel_name(bc1->chan),<br>+ target ? ast_channel_name(target) : "none");<br> break;<br> }<br> <br>@@ -265,7 +314,7 @@<br> static struct ast_frame *native_rtp_framehook(struct ast_channel *chan, struct ast_frame *f, enum ast_framehook_event event, void *data)<br> {<br> RAII_VAR(struct ast_bridge *, bridge, NULL, ao2_cleanup);<br>- struct native_rtp_bridge_data *native_data = data;<br>+ struct native_rtp_bridge_channel_data *native_data = data;<br> <br> if (!f || (event != AST_FRAMEHOOK_EVENT_WRITE)) {<br> return f;<br>@@ -439,10 +488,26 @@<br> return is_compatible;<br> }<br> <br>+static void hook_data_destructor(void *obj)<br>+{<br>+ struct native_rtp_bridge_channel_data *data = obj;<br>+ struct ast_rtp_instance *instance = data->audio_instance ?: data->video_instance;<br>+<br>+ ast_debug(2, "Destroying hook data on channel '%s', audio rtp instance '%p', "<br>+ "video rtp instance '%p'\n",<br>+ instance ? ast_rtp_instance_get_channel_id(instance) : "unknown",<br>+ data->audio_instance, data->video_instance);<br>+<br>+ ao2_cleanup(data->audio_instance);<br>+ ao2_cleanup(data->video_instance);<br>+}<br>+<br> /*! \brief Helper function which adds frame hook to bridge channel */<br> static int native_rtp_bridge_framehook_attach(struct ast_bridge_channel *bridge_channel)<br> {<br>- struct native_rtp_bridge_data *data = ao2_alloc(sizeof(*data), NULL);<br>+ struct native_rtp_bridge_channel_data *data = ao2_alloc(sizeof(*data), hook_data_destructor);<br>+ struct ast_rtp_glue *glue = ast_rtp_instance_get_glue(ast_channel_tech(bridge_channel->chan)->type);<br>+<br> static struct ast_framehook_interface hook = {<br> .version = AST_FRAMEHOOK_INTERFACE_VERSION,<br> .event_cb = native_rtp_framehook,<br>@@ -451,15 +516,21 @@<br> .disable_inheritance = 1,<br> };<br> <br>- if (!data) {<br>+ if (!data || !glue) {<br>+ ao2_cleanup(data);<br> return -1;<br> }<br> <br> ast_channel_lock(bridge_channel->chan);<br>+ // get_rtp_info bumps the instance refcount for us<br>+ glue->get_rtp_info(bridge_channel->chan, &data->audio_instance);<br>+ glue->get_vrtp_info(bridge_channel->chan, &data->video_instance);<br> hook.data = ao2_bump(data);<br> data->id = ast_framehook_attach(bridge_channel->chan, &hook);<br> ast_channel_unlock(bridge_channel->chan);<br> if (data->id < 0) {<br>+ ao2_ref(data->audio_instance, -1);<br>+ ao2_ref(data->video_instance, -1);<br> /* We need to drop both the reference we hold, and the one the framehook would hold */<br> ao2_ref(data, -2);<br> return -1;<br>@@ -473,7 +544,7 @@<br> /*! \brief Helper function which removes frame hook from bridge channel */<br> static void native_rtp_bridge_framehook_detach(struct ast_bridge_channel *bridge_channel)<br> {<br>- RAII_VAR(struct native_rtp_bridge_data *, data, bridge_channel->tech_pvt, ao2_cleanup);<br>+ struct native_rtp_bridge_channel_data *data = bridge_channel->tech_pvt;<br> <br> if (!data) {<br> return;<br>@@ -483,11 +554,14 @@<br> ast_framehook_detach(bridge_channel->chan, data->id);<br> data->detached = 1;<br> ast_channel_unlock(bridge_channel->chan);<br>+ ao2_cleanup(data);<br> bridge_channel->tech_pvt = NULL;<br> }<br> <br> static int native_rtp_bridge_join(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel)<br> {<br>+ ast_debug(2, "Channel '%s' is joining bridge '%s'\n",<br>+ ast_channel_name(bridge_channel->chan), bridge->uniqueid);<br> native_rtp_bridge_framehook_detach(bridge_channel);<br> if (native_rtp_bridge_framehook_attach(bridge_channel)) {<br> return -1;<br>@@ -497,15 +571,35 @@<br> return 0;<br> }<br> <br>+/*!<br>+ * \internal<br>+ * \brief For debugging bridge lifecycle only<br>+ */<br> static void native_rtp_bridge_unsuspend(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel)<br> {<br>+ ast_debug(2, "Channel '%s' is unsuspended back to bridge '%s'\n",<br>+ ast_channel_name(bridge_channel->chan), bridge->uniqueid);<br> native_rtp_bridge_join(bridge, bridge_channel);<br> }<br> <br> static void native_rtp_bridge_leave(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel)<br> {<br>- native_rtp_bridge_framehook_detach(bridge_channel);<br>+ ast_debug(2, "Channel '%s' is leaving bridge '%s'\n",<br>+ ast_channel_name(bridge_channel->chan), bridge->uniqueid);<br>+<br> native_rtp_bridge_stop(bridge, NULL);<br>+ native_rtp_bridge_framehook_detach(bridge_channel);<br>+}<br>+<br>+/*!<br>+ * \internal<br>+ * \brief For debugging bridge lifecycle only<br>+ */<br>+static void native_rtp_bridge_suspend(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel)<br>+{<br>+ ast_debug(2, "Channel '%s' is suspended from bridge '%s'\n",<br>+ ast_channel_name(bridge_channel->chan), bridge->uniqueid);<br>+ native_rtp_bridge_leave(bridge, bridge_channel);<br> }<br> <br> static int native_rtp_bridge_write(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel, struct ast_frame *frame)<br>@@ -547,10 +641,12 @@<br> .name = "native_rtp",<br> .capabilities = AST_BRIDGE_CAPABILITY_NATIVE,<br> .preference = AST_BRIDGE_PREFERENCE_BASE_NATIVE,<br>+ .create = native_rtp_bridge_create,<br>+ .destroy = native_rtp_bridge_destroy,<br> .join = native_rtp_bridge_join,<br> .unsuspend = native_rtp_bridge_unsuspend,<br> .leave = native_rtp_bridge_leave,<br>- .suspend = native_rtp_bridge_leave,<br>+ .suspend = native_rtp_bridge_suspend,<br> .write = native_rtp_bridge_write,<br> .compatible = native_rtp_bridge_compatible,<br> };<br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/5855">change 5855</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/5855"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 14 </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>
<div style="display:none"> Gerrit-Change-Id: I9e1ac49fa4af68d64826ccccd152593cf8cdb21a </div>
<div style="display:none"> Gerrit-Change-Number: 5855 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: George Joseph <gjoseph@digium.com> </div>