[Asterisk-code-review] bridge native rtp: Keep references to rtp instances on hook... (asterisk[master])
George Joseph
asteriskteam at digium.com
Thu Jun 15 19:29:43 CDT 2017
George Joseph has uploaded this change for review. ( https://gerrit.asterisk.org/5856
Change subject: bridge_native_rtp: Keep references to rtp instances on hook data
......................................................................
bridge_native_rtp: Keep references to rtp instances on hook data
There have been reports of deadlocks caused by an attempt to send
a frame to a channel's rtp instance after the channel has left the
native bridge and been destroyed. This patch effectively causes the
frame hook to keep a reference to both the audio and video rtp
instances until after the hook has been detached.
ASTERISK-26978 #close
Reported-by: Ross Beer
Change-Id: I9e1ac49fa4af68d64826ccccd152593cf8cdb21a
---
M bridges/bridge_native_rtp.c
1 file changed, 103 insertions(+), 7 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/56/5856/1
diff --git a/bridges/bridge_native_rtp.c b/bridges/bridge_native_rtp.c
index 4af93bf..5059df7 100644
--- a/bridges/bridge_native_rtp.c
+++ b/bridges/bridge_native_rtp.c
@@ -42,14 +42,19 @@
#include "asterisk/bridge.h"
#include "asterisk/bridge_technology.h"
#include "asterisk/frame.h"
+#include "asterisk/linkedlists.h"
#include "asterisk/rtp_engine.h"
/*! \brief Internal structure which contains information about bridged RTP channels */
-struct native_rtp_bridge_data {
+struct native_rtp_bridge_channel_data {
/*! \brief Framehook used to intercept certain control frames */
int id;
/*! \brief Set when this framehook has been detached */
unsigned int detached;
+ /*! \brief This channel's audio rtp instance */
+ struct ast_rtp_instance *audio_instance;
+ /*! \brief This channel's video rtp instance */
+ struct ast_rtp_instance *video_instance;
};
/*! \brief Internal helper function which gets all RTP information (glue and instances) relating to the given channels */
@@ -118,6 +123,26 @@
/*!
* \internal
+ * \brief For debugging bridge lifecycle only
+ */
+static int native_rtp_bridge_create(struct ast_bridge *bridge)
+{
+ ast_debug(2, "Bridge '%s' created\n", bridge->uniqueid);
+
+ return 0;
+}
+
+/*!
+ * \internal
+ * \brief For debugging bridge lifecycle only
+ */
+static void native_rtp_bridge_destroy(struct ast_bridge *bridge)
+{
+ ast_debug(2, "Bridge '%s' destroyed\n", bridge->uniqueid);
+}
+
+/*!
+ * \internal
* \brief Start native RTP bridging of two channels
*
* \param bridge The bridge that had native RTP bridging happening on it
@@ -141,8 +166,14 @@
RAII_VAR(struct ast_format_cap *, cap1, ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT), ao2_cleanup);
if (bc0 == bc1) {
+ ast_debug(2, "Bridge '%s' doesn't start when both channels are '%s'\n", bridge->uniqueid,
+ ast_channel_name(bc0->chan));
return;
}
+
+ ast_debug(2, "Bridge '%s' starting with channels '%s' and '%s' and target: '%s'\n", bridge->uniqueid,
+ ast_channel_name(bc0->chan), ast_channel_name(bc1->chan),
+ target ? ast_channel_name(target) : "none");
ast_channel_lock_both(bc0->chan, bc1->chan);
if (!bc0->suspended && !bc1->suspended) {
@@ -159,6 +190,8 @@
}
ast_rtp_instance_set_bridged(instance0, instance1);
ast_rtp_instance_set_bridged(instance1, instance0);
+ ast_debug(2, "Locally RTP bridged '%s' and '%s' in stack\n",
+ ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));
ast_verb(4, "Locally RTP bridged '%s' and '%s' in stack\n",
ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));
break;
@@ -175,6 +208,8 @@
if (!target) {
glue0->update_peer(bc0->chan, instance1, vinstance1, tinstance1, cap1, 0);
glue1->update_peer(bc1->chan, instance0, vinstance0, tinstance0, cap0, 0);
+ ast_debug(2, "Remotely bridged '%s' and '%s' - media will flow directly between them\n",
+ ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));
ast_verb(4, "Remotely bridged '%s' and '%s' - media will flow directly between them\n",
ast_channel_name(bc0->chan), ast_channel_name(bc1->chan));
} else {
@@ -192,6 +227,10 @@
}
break;
case AST_RTP_GLUE_RESULT_FORBID:
+ ast_log(LOG_ERROR, "FORBID returned while attempting to START bridge between '%s' and '%s' "
+ "with target '%s'\n",
+ ast_channel_name(bc0->chan), ast_channel_name(bc1->chan),
+ target ? ast_channel_name(target) : "none");
break;
}
@@ -211,8 +250,14 @@
RAII_VAR(struct ast_rtp_instance *, vinstance1, NULL, ao2_cleanup);
if (bc0 == bc1) {
+ ast_debug(2, "Bridge '%s' doesn't stop when both channels are '%s'\n", bridge->uniqueid,
+ ast_channel_name(bc0->chan));
return;
}
+
+ ast_debug(2, "Bridge '%s' stopping with channels '%s' and '%s'. Target: '%s'\n", bridge->uniqueid,
+ ast_channel_name(bc0->chan), ast_channel_name(bc1->chan),
+ target ? ast_channel_name(target) : "none");
ast_channel_lock_both(bc0->chan, bc1->chan);
native_type = native_rtp_bridge_get(bc0->chan, bc1->chan, &glue0, &glue1, &instance0, &instance1, &vinstance0, &vinstance1);
@@ -244,6 +289,10 @@
}
break;
case AST_RTP_GLUE_RESULT_FORBID:
+ ast_log(LOG_ERROR, "FORBID returned while attempting to STOP bridge between '%s' and '%s' "
+ "with target '%s'\n",
+ ast_channel_name(bc0->chan), ast_channel_name(bc1->chan),
+ target ? ast_channel_name(target) : "none");
break;
}
@@ -263,7 +312,7 @@
static struct ast_frame *native_rtp_framehook(struct ast_channel *chan, struct ast_frame *f, enum ast_framehook_event event, void *data)
{
RAII_VAR(struct ast_bridge *, bridge, NULL, ao2_cleanup);
- struct native_rtp_bridge_data *native_data = data;
+ struct native_rtp_bridge_channel_data *native_data = data;
if (!f || (event != AST_FRAMEHOOK_EVENT_WRITE)) {
return f;
@@ -437,10 +486,26 @@
return is_compatible;
}
+static void hook_data_destructor(void *obj)
+{
+ struct native_rtp_bridge_channel_data *data = obj;
+ struct ast_rtp_instance *instance = data->audio_instance ?: data->video_instance;
+
+ ast_debug(2, "Destroying hook data on channel '%s', audio rtp instance '%p', "
+ "video rtp instance '%p'\n",
+ instance ? ast_rtp_instance_get_channel_id(instance) : "unknown",
+ data->audio_instance, data->video_instance);
+
+ ao2_cleanup(data->audio_instance);
+ ao2_cleanup(data->video_instance);
+}
+
/*! \brief Helper function which adds frame hook to bridge channel */
static int native_rtp_bridge_framehook_attach(struct ast_bridge_channel *bridge_channel)
{
- struct native_rtp_bridge_data *data = ao2_alloc(sizeof(*data), NULL);
+ struct native_rtp_bridge_channel_data *data = ao2_alloc(sizeof(*data), hook_data_destructor);
+ struct ast_rtp_glue *glue = ast_rtp_instance_get_glue(ast_channel_tech(bridge_channel->chan)->type);
+
static struct ast_framehook_interface hook = {
.version = AST_FRAMEHOOK_INTERFACE_VERSION,
.event_cb = native_rtp_framehook,
@@ -449,15 +514,21 @@
.disable_inheritance = 1,
};
- if (!data) {
+ if (!data || !glue) {
+ ao2_cleanup(data);
return -1;
}
ast_channel_lock(bridge_channel->chan);
+ // get_rtp_info bumps the instance refcount for us
+ glue->get_rtp_info(bridge_channel->chan, &data->audio_instance);
+ glue->get_vrtp_info(bridge_channel->chan, &data->video_instance);
hook.data = ao2_bump(data);
data->id = ast_framehook_attach(bridge_channel->chan, &hook);
ast_channel_unlock(bridge_channel->chan);
if (data->id < 0) {
+ ao2_ref(data->audio_instance, -1);
+ ao2_ref(data->video_instance, -1);
/* We need to drop both the reference we hold, and the one the framehook would hold */
ao2_ref(data, -2);
return -1;
@@ -471,7 +542,7 @@
/*! \brief Helper function which removes frame hook from bridge channel */
static void native_rtp_bridge_framehook_detach(struct ast_bridge_channel *bridge_channel)
{
- RAII_VAR(struct native_rtp_bridge_data *, data, bridge_channel->tech_pvt, ao2_cleanup);
+ struct native_rtp_bridge_channel_data *data = bridge_channel->tech_pvt;
if (!data) {
return;
@@ -481,11 +552,14 @@
ast_framehook_detach(bridge_channel->chan, data->id);
data->detached = 1;
ast_channel_unlock(bridge_channel->chan);
+ ao2_cleanup(data);
bridge_channel->tech_pvt = NULL;
}
static int native_rtp_bridge_join(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel)
{
+ ast_debug(2, "Channel '%s' is joining bridge '%s'\n",
+ ast_channel_name(bridge_channel->chan), bridge->uniqueid);
native_rtp_bridge_framehook_detach(bridge_channel);
if (native_rtp_bridge_framehook_attach(bridge_channel)) {
return -1;
@@ -495,15 +569,35 @@
return 0;
}
+/*!
+ * \internal
+ * \brief For debugging bridge lifecycle only
+ */
static void native_rtp_bridge_unsuspend(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel)
{
+ ast_debug(2, "Channel '%s' is unsuspended back to bridge '%s'\n",
+ ast_channel_name(bridge_channel->chan), bridge->uniqueid);
native_rtp_bridge_join(bridge, bridge_channel);
}
static void native_rtp_bridge_leave(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel)
{
- native_rtp_bridge_framehook_detach(bridge_channel);
+ ast_debug(2, "Channel '%s' is leaving bridge '%s'\n",
+ ast_channel_name(bridge_channel->chan), bridge->uniqueid);
+
native_rtp_bridge_stop(bridge, NULL);
+ native_rtp_bridge_framehook_detach(bridge_channel);
+}
+
+/*!
+ * \internal
+ * \brief For debugging bridge lifecycle only
+ */
+static void native_rtp_bridge_suspend(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel)
+{
+ ast_debug(2, "Channel '%s' is suspended from bridge '%s'\n",
+ ast_channel_name(bridge_channel->chan), bridge->uniqueid);
+ native_rtp_bridge_leave(bridge, bridge_channel);
}
static int native_rtp_bridge_write(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel, struct ast_frame *frame)
@@ -545,10 +639,12 @@
.name = "native_rtp",
.capabilities = AST_BRIDGE_CAPABILITY_NATIVE,
.preference = AST_BRIDGE_PREFERENCE_BASE_NATIVE,
+ .create = native_rtp_bridge_create,
+ .destroy = native_rtp_bridge_destroy,
.join = native_rtp_bridge_join,
.unsuspend = native_rtp_bridge_unsuspend,
.leave = native_rtp_bridge_leave,
- .suspend = native_rtp_bridge_leave,
+ .suspend = native_rtp_bridge_suspend,
.write = native_rtp_bridge_write,
.compatible = native_rtp_bridge_compatible,
};
--
To view, visit https://gerrit.asterisk.org/5856
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9e1ac49fa4af68d64826ccccd152593cf8cdb21a
Gerrit-Change-Number: 5856
Gerrit-PatchSet: 1
Gerrit-Owner: George Joseph <gjoseph at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170615/48f1c552/attachment-0001.html>
More information about the asterisk-code-review
mailing list