[asterisk-commits] kharwell: trunk r403768 - in /trunk: ./ bridges/ channels/ include/asterisk/ ...

SVN commits to the Asterisk project asterisk-commits at lists.digium.com
Fri Dec 13 12:33:27 CST 2013


Author: kharwell
Date: Fri Dec 13 12:33:25 2013
New Revision: 403768

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=403768
Log:
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

Modified:
    trunk/   (props changed)
    trunk/bridges/bridge_native_rtp.c
    trunk/channels/chan_pjsip.c
    trunk/channels/chan_sip.c
    trunk/include/asterisk/channel.h
    trunk/main/channel.c

Propchange: trunk/
------------------------------------------------------------------------------
--- branch-12-merged (original)
+++ branch-12-merged Fri Dec 13 12:33:25 2013
@@ -1,1 +1,1 @@
-/branches/12:1-398558,398560-398577,398579-399305,399307-401390,401392-403290,403292-403398,403435,403458,403510,403527,403541-403542,403545,403559,403587,403616,403618,403639,403680,403705,403713,403736,403749,403752
+/branches/12:1-398558,398560-398577,398579-399305,399307-401390,401392-403290,403292-403398,403435,403458,403510,403527,403541-403542,403545,403559,403587,403616,403618,403639,403680,403705,403713,403736,403749,403752,403767

Modified: trunk/bridges/bridge_native_rtp.c
URL: http://svnview.digium.com/svn/asterisk/trunk/bridges/bridge_native_rtp.c?view=diff&rev=403768&r1=403767&r2=403768
==============================================================================
--- trunk/bridges/bridge_native_rtp.c (original)
+++ trunk/bridges/bridge_native_rtp.c Fri Dec 13 12:33:25 2013
@@ -112,7 +112,16 @@
 	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,17 +137,11 @@
 	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:
@@ -155,6 +158,12 @@
 		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 @@
 		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 @@
 		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 @@
 
 	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 @@
 		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 @@
 		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)

Modified: trunk/channels/chan_pjsip.c
URL: http://svnview.digium.com/svn/asterisk/trunk/channels/chan_pjsip.c?view=diff&rev=403768&r1=403767&r2=403768
==============================================================================
--- trunk/channels/chan_pjsip.c (original)
+++ trunk/channels/chan_pjsip.c Fri Dec 13 12:33:25 2013
@@ -292,14 +292,11 @@
 	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;

Modified: trunk/channels/chan_sip.c
URL: http://svnview.digium.com/svn/asterisk/trunk/channels/chan_sip.c?view=diff&rev=403768&r1=403767&r2=403768
==============================================================================
--- trunk/channels/chan_sip.c (original)
+++ trunk/channels/chan_sip.c Fri Dec 13 12:33:25 2013
@@ -32667,11 +32667,8 @@
 	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 @@
 		/* 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 @@
 		!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 @@
 	*/
 	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 @@
 		 */
 		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 @@
 	/* Reset lastrtprx timer */
 	p->lastrtprx = p->lastrtptx = time(NULL);
 	sip_pvt_unlock(p);
-	ast_channel_unlock(chan);
 	return 0;
 }
 

Modified: trunk/include/asterisk/channel.h
URL: http://svnview.digium.com/svn/asterisk/trunk/include/asterisk/channel.h?view=diff&rev=403768&r1=403767&r2=403768
==============================================================================
--- trunk/include/asterisk/channel.h (original)
+++ trunk/include/asterisk/channel.h Fri Dec 13 12:33:25 2013
@@ -4214,6 +4214,8 @@
  * \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.
  */

Modified: trunk/main/channel.c
URL: http://svnview.digium.com/svn/asterisk/trunk/main/channel.c?view=diff&rev=403768&r1=403767&r2=403768
==============================================================================
--- trunk/main/channel.c (original)
+++ trunk/main/channel.c Fri Dec 13 12:33:25 2013
@@ -6869,13 +6869,11 @@
 	}
 
 	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);
 




More information about the asterisk-commits mailing list