[asterisk-commits] mjordan: trunk r350551 - in /trunk: ./ apps/ bridges/ channels/ include/aster...

SVN commits to the Asterisk project asterisk-commits at lists.digium.com
Fri Jan 13 10:48:10 CST 2012


Author: mjordan
Date: Fri Jan 13 10:48:06 2012
New Revision: 350551

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=350551
Log:
Fix crash from bridge channel hangup race condition in ConfBridge

This patch addresses two issues in ConfBridge and the channel bridge layer:
1. It fixes a race condition wherein the bridge channel could be hung up
2. It removes the deadlock avoidance from the bridging layer and makes the
   bridge_pvt an ao2 ref counted object

Patch by David Vossel (mjordan was merely the commit monkey)

(issue ASTERISK-18988)
(closes issue ASTERISK-18885)
Reported by: Dmitry Melekhov
Tested by: Matt Jordan
Patches: chan_bridge_cleanup_v.diff uploaded by David Vossel (license 5628)

(closes issue ASTERISK-19100)
Reported by: Matt Jordan
Tested by: Matt Jordan

Review: https://reviewboard.asterisk.org/r/1654/
........

Merged revisions 350550 from http://svn.asterisk.org/svn/asterisk/branches/10

Modified:
    trunk/   (props changed)
    trunk/apps/app_confbridge.c
    trunk/bridges/bridge_builtin_features.c
    trunk/channels/chan_bridge.c
    trunk/include/asterisk/bridging.h
    trunk/main/bridging.c

Propchange: trunk/
------------------------------------------------------------------------------
Binary property 'branch-10-merged' - no diff available.

Modified: trunk/apps/app_confbridge.c
URL: http://svnview.digium.com/svn/asterisk/trunk/apps/app_confbridge.c?view=diff&rev=350551&r1=350550&r2=350551
==============================================================================
--- trunk/apps/app_confbridge.c (original)
+++ trunk/apps/app_confbridge.c Fri Jan 13 10:48:06 2012
@@ -874,7 +874,9 @@
 
 	if (conference_bridge->playback_chan) {
 		struct ast_channel *underlying_channel = conference_bridge->playback_chan->tech->bridged_channel(conference_bridge->playback_chan, NULL);
-		ast_hangup(underlying_channel);
+		if (underlying_channel) {
+			ast_hangup(underlying_channel);
+		}
 		ast_hangup(conference_bridge->playback_chan);
 		conference_bridge->playback_chan = NULL;
 	}
@@ -1155,7 +1157,7 @@
 	} else {
 		/* Channel was already available so we just need to add it back into the bridge */
 		underlying_channel = conference_bridge->playback_chan->tech->bridged_channel(conference_bridge->playback_chan, NULL);
-		ast_bridge_impart(conference_bridge->bridge, underlying_channel, NULL, NULL);
+		ast_bridge_impart(conference_bridge->bridge, underlying_channel, NULL, NULL, 0);
 	}
 
 	/* The channel is all under our control, in goes the prompt */

Modified: trunk/bridges/bridge_builtin_features.c
URL: http://svnview.digium.com/svn/asterisk/trunk/bridges/bridge_builtin_features.c?view=diff&rev=350551&r1=350550&r2=350551
==============================================================================
--- trunk/bridges/bridge_builtin_features.c (original)
+++ trunk/bridges/bridge_builtin_features.c Fri Jan 13 10:48:06 2012
@@ -123,7 +123,7 @@
 	}
 
 	/* This is sort of the fun part. We impart the above channel onto the bridge, and have it take our place. */
-	ast_bridge_impart(bridge, chan, bridge_channel->chan, NULL);
+	ast_bridge_impart(bridge, chan, bridge_channel->chan, NULL, 1);
 
 	return 0;
 }
@@ -200,7 +200,7 @@
 	ast_bridge_features_set_flag(&called_features, AST_BRIDGE_FLAG_DISSOLVE);
 
 	/* This is how this is going down, we are imparting the channel we called above into this bridge first */
-	ast_bridge_impart(attended_bridge, chan, NULL, &called_features);
+	ast_bridge_impart(attended_bridge, chan, NULL, &called_features, 1);
 
 	/* Before we join setup a features structure with the hangup option, just in case they want to use DTMF */
 	ast_bridge_features_init(&caller_features);
@@ -222,9 +222,9 @@
 		/* If the user wants to turn this into a threeway transfer then do so, otherwise they take our place */
 		if (attended_bridge_result == AST_BRIDGE_CHANNEL_STATE_DEPART) {
 			/* We want to impart them upon the bridge and just have us return to it as normal */
-			ast_bridge_impart(bridge, chan, NULL, NULL);
+			ast_bridge_impart(bridge, chan, NULL, NULL, 1);
 		} else {
-			ast_bridge_impart(bridge, chan, bridge_channel->chan, NULL);
+			ast_bridge_impart(bridge, chan, bridge_channel->chan, NULL, 1);
 		}
 	} else {
 		ast_stream_and_wait(bridge_channel->chan, "beeperr", AST_DIGIT_ANY);

Modified: trunk/channels/chan_bridge.c
URL: http://svnview.digium.com/svn/asterisk/trunk/channels/chan_bridge.c?view=diff&rev=350551&r1=350550&r2=350551
==============================================================================
--- trunk/channels/chan_bridge.c (original)
+++ trunk/channels/chan_bridge.c Fri Jan 13 10:48:06 2012
@@ -49,6 +49,7 @@
 #include "asterisk/cli.h"
 #include "asterisk/app.h"
 #include "asterisk/bridging.h"
+#include "asterisk/astobj2.h"
 
 static struct ast_channel *bridge_request(const char *type, struct ast_format_cap *cap, const struct ast_channel *requestor, void *data, int *cause);
 static int bridge_call(struct ast_channel *ast, char *dest, int timeout);
@@ -71,7 +72,6 @@
 };
 
 struct bridge_pvt {
-	ast_mutex_t lock;           /*!< Lock that protects this structure */
 	struct ast_channel *input;  /*!< Input channel - talking to source */
 	struct ast_channel *output; /*!< Output channel - talking to bridge */
 };
@@ -93,27 +93,24 @@
 static int bridge_write(struct ast_channel *ast, struct ast_frame *f)
 {
 	struct bridge_pvt *p = ast->tech_pvt;
-	struct ast_channel *other;
-
-	ast_mutex_lock(&p->lock);
-
-	other = (p->input == ast ? p->output : p->input);
-
-	while (other && ast_channel_trylock(other)) {
-		ast_mutex_unlock(&p->lock);
-		do {
-			CHANNEL_DEADLOCK_AVOIDANCE(ast);
-		} while (ast_mutex_trylock(&p->lock));
-		other = (p->input == ast ? p->output : p->input);
-	}
-
-	/* We basically queue the frame up on the other channel if present */
+	struct ast_channel *other = NULL;
+
+	ao2_lock(p);
+	/* only write frames to output. */
+	if (p->input == ast) {
+		other = p->output;
+		if (other) {
+			ast_channel_ref(other);
+		}
+	}
+	ao2_unlock(p);
+
 	if (other) {
+		ast_channel_unlock(ast);
 		ast_queue_frame(other, f);
-		ast_channel_unlock(other);
-	}
-
-	ast_mutex_unlock(&p->lock);
+		ast_channel_lock(ast);
+		other = ast_channel_unref(other);
+	}
 
 	return 0;
 }
@@ -129,31 +126,9 @@
 	}
 
 	/* Impart the output channel upon the given bridge of the input channel */
-	ast_bridge_impart(p->input->bridge, p->output, NULL, NULL);
-
-	return 0;
-}
-
-/*! \brief Helper function to not deadlock when queueing the hangup frame */
-static void bridge_queue_hangup(struct bridge_pvt *p, struct ast_channel *us)
-{
-	struct ast_channel *other = (p->input == us ? p->output : p->input);
-
-	while (other && ast_channel_trylock(other)) {
-		ast_mutex_unlock(&p->lock);
-		do {
-			CHANNEL_DEADLOCK_AVOIDANCE(us);
-		} while (ast_mutex_trylock(&p->lock));
-		other = (p->input == us ? p->output : p->input);
-	}
-
-	/* We basically queue the frame up on the other channel if present */
-	if (other) {
-		ast_queue_hangup(other);
-		ast_channel_unlock(other);
-	}
-
-	return;
+	ast_bridge_impart(p->input->bridge, p->output, NULL, NULL, 0);
+
+	return 0;
 }
 
 /*! \brief Called when a channel should be hung up */
@@ -161,32 +136,20 @@
 {
 	struct bridge_pvt *p = ast->tech_pvt;
 
-	ast_mutex_lock(&p->lock);
-
-	/* Figure out which channel this is... and set it to NULL as it has gone, but also queue up a hangup frame. */
+	if (!p) {
+		return 0;
+	}
+
+	ao2_lock(p);
 	if (p->input == ast) {
-		if (p->output) {
-			bridge_queue_hangup(p, ast);
-		}
 		p->input = NULL;
 	} else if (p->output == ast) {
-		if (p->input) {
-			bridge_queue_hangup(p, ast);
-		}
 		p->output = NULL;
 	}
-
-	/* Deal with the Asterisk portion of it */
+	ao2_unlock(p);
+
 	ast->tech_pvt = NULL;
-
-	/* If both sides have been terminated free the structure and be done with things */
-	if (!p->input && !p->output) {
-		ast_mutex_unlock(&p->lock);
-		ast_mutex_destroy(&p->lock);
-		ast_free(p);
-	} else {
-		ast_mutex_unlock(&p->lock);
-	}
+	ao2_ref(p, -1);
 
 	return 0;
 }
@@ -198,26 +161,25 @@
 	struct ast_format slin;
 
 	/* Try to allocate memory for our very minimal pvt structure */
-	if (!(p = ast_calloc(1, sizeof(*p)))) {
+	if (!(p = ao2_alloc(sizeof(*p), NULL))) {
 		return NULL;
 	}
 
 	/* Try to grab two Asterisk channels to use as input and output channels */
 	if (!(p->input = ast_channel_alloc(1, AST_STATE_UP, 0, 0, "", "", "", requestor ? requestor->linkedid : NULL, 0, "Bridge/%p-input", p))) {
-		ast_free(p);
+		ao2_ref(p, -1);
 		return NULL;
 	}
 	if (!(p->output = ast_channel_alloc(1, AST_STATE_UP, 0, 0, "", "", "", requestor ? requestor->linkedid : NULL, 0, "Bridge/%p-output", p))) {
 		p->input = ast_channel_release(p->input);
-		ast_free(p);
+		ao2_ref(p, -1);
 		return NULL;
 	}
-
-	/* Setup the lock on the pvt structure, we will need that */
-	ast_mutex_init(&p->lock);
 
 	/* Setup parameters on both new channels */
 	p->input->tech = p->output->tech = &bridge_tech;
+
+	ao2_ref(p, 2);
 	p->input->tech_pvt = p->output->tech_pvt = p;
 
 	ast_format_set(&slin, AST_FORMAT_SLINEAR, 0);
@@ -236,6 +198,8 @@
 	ast_answer(p->output);
 	ast_answer(p->input);
 
+	/* remove the reference from the alloc. The channels now own the pvt. */
+	ao2_ref(p, -1);
 	return p->input;
 }
 

Modified: trunk/include/asterisk/bridging.h
URL: http://svnview.digium.com/svn/asterisk/trunk/include/asterisk/bridging.h?view=diff&rev=350551&r1=350550&r2=350551
==============================================================================
--- trunk/include/asterisk/bridging.h (original)
+++ trunk/include/asterisk/bridging.h Fri Jan 13 10:48:06 2012
@@ -156,6 +156,8 @@
 	int fds[4];
 	/*! Bit to indicate whether the channel is suspended from the bridge or not */
 	unsigned int suspended:1;
+	/*! Bit to indicate if a imparted channel is allowed to get hungup after leaving the bridge by the bridging core. */
+	unsigned int allow_impart_hangup:1;
 	/*! Features structure for features that are specific to this channel */
 	struct ast_bridge_features *features;
 	/*! Technology optimization parameters used by bridging technologies capable of
@@ -339,14 +341,15 @@
  * \param chan Channel to impart
  * \param swap Channel to swap out if swapping
  * \param features Bridge features structure
- *
- * \retval 0 on success
- * \retval -1 on failure
- *
- * Example usage:
- *
- * \code
- * ast_bridge_impart(bridge, chan, NULL, NULL);
+ * \param allow_hangup  Indicates if the bridge thread should manage hanging up of the channel or not.
+ *
+ * \retval 0 on success
+ * \retval -1 on failure
+ *
+ * Example usage:
+ *
+ * \code
+ * ast_bridge_impart(bridge, chan, NULL, NULL, 0);
  * \endcode
  *
  * This adds a channel pointed to by the chan pointer to the bridge pointed to by
@@ -360,7 +363,7 @@
  * If channel specific features are enabled a pointer to the features structure
  * can be specified in the features parameter.
  */
-int ast_bridge_impart(struct ast_bridge *bridge, struct ast_channel *chan, struct ast_channel *swap, struct ast_bridge_features *features);
+int ast_bridge_impart(struct ast_bridge *bridge, struct ast_channel *chan, struct ast_channel *swap, struct ast_bridge_features *features, int allow_hangup);
 
 /*! \brief Depart a channel from a bridge
  *

Modified: trunk/main/bridging.c
URL: http://svnview.digium.com/svn/asterisk/trunk/main/bridging.c?view=diff&rev=350551&r1=350550&r2=350551
==============================================================================
--- trunk/main/bridging.c (original)
+++ trunk/main/bridging.c Fri Jan 13 10:48:06 2012
@@ -1121,7 +1121,7 @@
 	state = bridge_channel_join(bridge_channel);
 
 	/* If no other thread is going to take the channel then hang it up, or else we would have to service it until something else came along */
-	if (state == AST_BRIDGE_CHANNEL_STATE_END || state == AST_BRIDGE_CHANNEL_STATE_HANGUP) {
+	if (bridge_channel->allow_impart_hangup && (state == AST_BRIDGE_CHANNEL_STATE_END || state == AST_BRIDGE_CHANNEL_STATE_HANGUP)) {
 		ast_hangup(bridge_channel->chan);
 	}
 
@@ -1137,7 +1137,7 @@
 	return NULL;
 }
 
-int ast_bridge_impart(struct ast_bridge *bridge, struct ast_channel *chan, struct ast_channel *swap, struct ast_bridge_features *features)
+int ast_bridge_impart(struct ast_bridge *bridge, struct ast_channel *chan, struct ast_channel *swap, struct ast_bridge_features *features, int allow_hangup)
 {
 	struct ast_bridge_channel *bridge_channel = bridge_channel_alloc(bridge);
 	/* Try to allocate a structure for the bridge channel */
@@ -1149,6 +1149,8 @@
 	bridge_channel->chan = chan;
 	bridge_channel->swap = swap;
 	bridge_channel->features = features;
+	bridge_channel->allow_impart_hangup = allow_hangup;
+
 
 	/* Actually create the thread that will handle the channel */
 	if (ast_pthread_create(&bridge_channel->thread, NULL, bridge_channel_thread, bridge_channel)) {




More information about the asterisk-commits mailing list