From 84e1790beb1c28f762fa991f99d4a41ede2c3935 Mon Sep 17 00:00:00 2001
From: Kevin Harwell <kharwell@digium.com>
Date: Fri, 13 Dec 2013 18:33:25 +0000
Subject: [PATCH] bridge_native_rtp: Deadlock during 4-way conference creation
The change contains a slightly adjusted patch that was on the issue
(submitted by kmoore). A fix was made by adding in a bridge lock
while calling bridge_start/stop from the framehook callback. Since
the framehook callback is not called from the bridging core the bridge
is not locked, but needs to be before calling bridge_start.
(closes issue ASTERISK-22749)
Reported by: Kinsey Moore
Review: https://reviewboard.asterisk.org/r/3066/
Patches:
lock_inversion.diff uploaded by kmoore (license 6273)
........
Merged revisions 403767 from http://svn.asterisk.org/svn/asterisk/branches/12
git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@403768 65c4cc65-6c06-0410-ace0-fbb531ad65f3
---
bridges/bridge_native_rtp.c | 48 +++++++++++++++++++++++++++----------
channels/chan_pjsip.c | 5 +---
channels/chan_sip.c | 9 -------
include/asterisk/channel.h | 2 ++
main/channel.c | 4 +---
5 files changed, 39 insertions(+), 29 deletions(-)
diff --git a/bridges/bridge_native_rtp.c b/bridges/bridge_native_rtp.c
index 27d8d77cac..7edcfd71aa 100644
--- a/bridges/bridge_native_rtp.c
+++ b/bridges/bridge_native_rtp.c
@@ -112,7 +112,16 @@ static enum ast_rtp_glue_result native_rtp_bridge_get(struct ast_channel *c0, st
return audio_glue0_res;
}
-static int native_rtp_bridge_start(struct ast_bridge *bridge, struct ast_channel *target)
+/*!
+ * \internal
+ * \brief Start native RTP bridging of two channels
+ *
+ * \param bridge The bridge that had native RTP bridging happening on it
+ * \param target If remote RTP bridging, the channel that is unheld.
+ *
+ * \note Bridge must be locked when calling this function.
+ */
+static void native_rtp_bridge_start(struct ast_bridge *bridge, struct ast_channel *target)
{
struct ast_bridge_channel *c0 = AST_LIST_FIRST(&bridge->channels);
struct ast_bridge_channel *c1 = AST_LIST_LAST(&bridge->channels);
@@ -128,18 +137,12 @@ static int native_rtp_bridge_start(struct ast_bridge *bridge, struct ast_channel
RAII_VAR(struct ast_format_cap *, cap1, ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_NOLOCK), ast_format_cap_destroy);
if (c0 == c1) {
- return 0;
+ return;
}
+ ast_channel_lock_both(c0->chan, c1->chan);
native_type = native_rtp_bridge_get(c0->chan, c1->chan, &glue0, &glue1, &instance0, &instance1, &vinstance0, &vinstance1);
- if (glue0->get_codec) {
- glue0->get_codec(c0->chan, cap0);
- }
- if (glue1->get_codec) {
- glue1->get_codec(c1->chan, cap1);
- }
-
switch (native_type) {
case AST_RTP_GLUE_RESULT_LOCAL:
if (ast_rtp_instance_get_engine(instance0)->local_bridge) {
@@ -155,6 +158,12 @@ static int native_rtp_bridge_start(struct ast_bridge *bridge, struct ast_channel
break;
case AST_RTP_GLUE_RESULT_REMOTE:
+ if (glue0->get_codec) {
+ glue0->get_codec(c0->chan, cap0);
+ }
+ if (glue1->get_codec) {
+ glue1->get_codec(c1->chan, cap1);
+ }
/* If we have a target, it's the channel that received the UNHOLD or UPDATE_RTP_PEER frame and was told to resume */
if (!target) {
@@ -180,7 +189,8 @@ static int native_rtp_bridge_start(struct ast_bridge *bridge, struct ast_channel
break;
}
- return 0;
+ ast_channel_unlock(c0->chan);
+ ast_channel_unlock(c1->chan);
}
static void native_rtp_bridge_stop(struct ast_bridge *bridge, struct ast_channel *target)
@@ -202,6 +212,7 @@ static void native_rtp_bridge_stop(struct ast_bridge *bridge, struct ast_channel
return;
}
+ ast_channel_lock_both(c0->chan, c1->chan);
native_type = native_rtp_bridge_get(c0->chan, c1->chan, &glue0, &glue1, &instance0, &instance1, &vinstance0, &vinstance1);
switch (native_type) {
@@ -241,6 +252,9 @@ static void native_rtp_bridge_stop(struct ast_bridge *bridge, struct ast_channel
ast_debug(2, "Discontinued RTP bridging of '%s' and '%s' - media will flow through Asterisk core\n",
ast_channel_name(c0->chan), ast_channel_name(c1->chan));
+
+ ast_channel_unlock(c0->chan);
+ ast_channel_unlock(c1->chan);
}
/*! \brief Frame hook that is called to intercept hold/unhold */
@@ -252,16 +266,23 @@ static struct ast_frame *native_rtp_framehook(struct ast_channel *chan, struct a
return f;
}
- ast_channel_lock(chan);
bridge = ast_channel_get_bridge(chan);
- ast_channel_unlock(chan);
if (bridge) {
+ /* native_rtp_bridge_start/stop are not being called from bridging
+ core so we need to lock the bridge prior to calling these functions
+ Unfortunately that means unlocking the channel, but as it
+ should not be modified this should be okay...hopefully */
+ ast_channel_unlock(chan);
+ ast_bridge_lock(bridge);
if (f->subclass.integer == AST_CONTROL_HOLD) {
native_rtp_bridge_stop(bridge, chan);
} else if ((f->subclass.integer == AST_CONTROL_UNHOLD) || (f->subclass.integer == AST_CONTROL_UPDATE_RTP_PEER)) {
native_rtp_bridge_start(bridge, chan);
}
+ ast_bridge_unlock(bridge);
+ ast_channel_lock(chan);
+
}
return f;
@@ -412,7 +433,8 @@ static int native_rtp_bridge_join(struct ast_bridge *bridge, struct ast_bridge_c
return -1;
}
- return native_rtp_bridge_start(bridge, NULL);
+ native_rtp_bridge_start(bridge, NULL);
+ return 0;
}
static void native_rtp_bridge_unsuspend(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel)
diff --git a/channels/chan_pjsip.c b/channels/chan_pjsip.c
index 8db3f094fc..21a79f0edb 100644
--- a/channels/chan_pjsip.c
+++ b/channels/chan_pjsip.c
@@ -292,14 +292,11 @@ static int chan_pjsip_set_rtp_peer(struct ast_channel *chan,
struct chan_pjsip_pvt *pvt = channel->pvt;
struct ast_sip_session *session = channel->session;
int changed = 0;
- struct ast_channel *bridge_peer;
/* Don't try to do any direct media shenanigans on early bridges */
- bridge_peer = ast_channel_bridge_peer(chan);
- if ((rtp || vrtp || tpeer) && !bridge_peer) {
+ if ((rtp || vrtp || tpeer) && !ast_channel_is_bridged(chan)) {
return 0;
}
- ast_channel_cleanup(bridge_peer);
if (nat_active && session->endpoint->media.direct_media.disable_on_nat) {
return 0;
diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index 5d729cd020..a7649127e7 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -32667,11 +32667,8 @@ static int sip_set_rtp_peer(struct ast_channel *chan, struct ast_rtp_instance *i
struct sip_pvt *p;
int changed = 0;
- /* Lock the channel and the private safely. */
- ast_channel_lock(chan);
p = ast_channel_tech_pvt(chan);
if (!p) {
- ast_channel_unlock(chan);
return -1;
}
sip_pvt_lock(p);
@@ -32679,7 +32676,6 @@ static int sip_set_rtp_peer(struct ast_channel *chan, struct ast_rtp_instance *i
/* I suppose it could be argued that if this happens it is a bug. */
ast_debug(1, "The private is not owned by channel %s anymore.\n", ast_channel_name(chan));
sip_pvt_unlock(p);
- ast_channel_unlock(chan);
return 0;
}
@@ -32688,14 +32684,12 @@ static int sip_set_rtp_peer(struct ast_channel *chan, struct ast_rtp_instance *i
!ast_channel_is_bridged(chan) &&
!sip_cfg.directrtpsetup) {
sip_pvt_unlock(p);
- ast_channel_unlock(chan);
return 0;
}
if (p->alreadygone) {
/* If we're destroyed, don't bother */
sip_pvt_unlock(p);
- ast_channel_unlock(chan);
return 0;
}
@@ -32704,7 +32698,6 @@ static int sip_set_rtp_peer(struct ast_channel *chan, struct ast_rtp_instance *i
*/
if (nat_active && !ast_test_flag(&p->flags[0], SIP_DIRECT_MEDIA_NAT)) {
sip_pvt_unlock(p);
- ast_channel_unlock(chan);
return 0;
}
@@ -32770,7 +32763,6 @@ static int sip_set_rtp_peer(struct ast_channel *chan, struct ast_rtp_instance *i
*/
ast_clear_flag(&p->flags[2], SIP_PAGE3_DIRECT_MEDIA_OUTGOING);
sip_pvt_unlock(p);
- ast_channel_unlock(chan);
return 0;
}
@@ -32791,7 +32783,6 @@ static int sip_set_rtp_peer(struct ast_channel *chan, struct ast_rtp_instance *i
/* Reset lastrtprx timer */
p->lastrtprx = p->lastrtptx = time(NULL);
sip_pvt_unlock(p);
- ast_channel_unlock(chan);
return 0;
}
diff --git a/include/asterisk/channel.h b/include/asterisk/channel.h
index 7db2b3c3a2..03d9aef68a 100644
--- a/include/asterisk/channel.h
+++ b/include/asterisk/channel.h
@@ -4214,6 +4214,8 @@ int ast_channel_is_bridged(const struct ast_channel *chan);
* \note The returned peer channel is the current peer in the
* bridge when called.
*
+ * \note Absolutely _NO_ channel locks should be held when calling this function.
+ *
* \retval NULL Channel not in a bridge or the bridge is not two-party.
* \retval non-NULL Reffed peer channel at time of calling.
*/
diff --git a/main/channel.c b/main/channel.c
index 58b50c820b..d1643658e1 100644
--- a/main/channel.c
+++ b/main/channel.c
@@ -6869,13 +6869,11 @@ void ast_do_masquerade(struct ast_channel *original)
}
ast_debug(1, "Done Masquerading %s (%d)\n", ast_channel_name(original), ast_channel_state(original));
+ ast_channel_unlock(original);
if ((bridged = ast_channel_bridge_peer(original))) {
- ast_channel_unlock(original);
ast_indicate(bridged, AST_CONTROL_SRCCHANGE);
ast_channel_unref(bridged);
- } else {
- ast_channel_unlock(original);
}
ast_indicate(original, AST_CONTROL_SRCCHANGE);
--
GitLab