kenmirrors/asterisk
1
0
Fork 0
mirror of https://github.com/asterisk/asterisk.git synced 2025-10-13 00:04:53 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix crash from bridge channel hangup race condition in ConfBridge

Browse Source 
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


git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@350551 65c4cc65-6c06-0410-ace0-fbb531ad65f3
This commit is contained in:
Matthew Jordan
2012-01-13 16:48:06 +00:00
parent 19a9761084
commit a8276fe8ef
5 changed files with 44 additions and 73 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
apps/app_confbridge.c
View File

@@ -874,7 +874,9 @@ static void destroy_conference_bridge(void *obj)
if (conference_bridge->playback_chan) { if (conference_bridge->playback_chan) {
struct ast_channel *underlying_channel = conference_bridge->playback_chan->tech->bridged_channel(conference_bridge->playback_chan, NULL); 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); ast_hangup(conference_bridge->playback_chan);
conference_bridge->playback_chan = NULL; conference_bridge->playback_chan = NULL;
} }
@@ -1155,7 +1157,7 @@ static int play_sound_helper(struct conference_bridge *conference_bridge, const
} else { } else {
/* Channel was already available so we just need to add it back into the bridge */ /* 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); 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 */ /* The channel is all under our control, in goes the prompt */

8
bridges/bridge_builtin_features.c
View File

@@ -123,7 +123,7 @@ static int feature_blind_transfer(struct ast_bridge *bridge, struct ast_bridge_c
} }
/* This is sort of the fun part. We impart the above channel onto the bridge, and have it take our place. */ /* 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; return 0;
} }
@@ -200,7 +200,7 @@ static int feature_attended_transfer(struct ast_bridge *bridge, struct ast_bridg
ast_bridge_features_set_flag(&called_features, AST_BRIDGE_FLAG_DISSOLVE); 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 */ /* 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 */ /* 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); ast_bridge_features_init(&caller_features);
@@ -222,9 +222,9 @@ static int feature_attended_transfer(struct ast_bridge *bridge, struct ast_bridg
/* If the user wants to turn this into a threeway transfer then do so, otherwise they take our place */ /* 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) { 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 */ /* 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 { } else {
ast_bridge_impart(bridge, chan, bridge_channel->chan, NULL); ast_bridge_impart(bridge, chan, bridge_channel->chan, NULL, 1);
} }
} else { } else {
ast_stream_and_wait(bridge_channel->chan, "beeperr", AST_DIGIT_ANY); ast_stream_and_wait(bridge_channel->chan, "beeperr", AST_DIGIT_ANY);

90
channels/chan_bridge.c
View File

@@ -49,6 +49,7 @@ ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
#include "asterisk/cli.h" #include "asterisk/cli.h"
#include "asterisk/app.h" #include "asterisk/app.h"
#include "asterisk/bridging.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 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); static int bridge_call(struct ast_channel *ast, char *dest, int timeout);
@@ -71,7 +72,6 @@ static struct ast_channel_tech bridge_tech = {
}; };
struct bridge_pvt { struct bridge_pvt {
ast_mutex_t lock; /*!< Lock that protects this structure */
struct ast_channel *input; /*!< Input channel - talking to source */ struct ast_channel *input; /*!< Input channel - talking to source */
struct ast_channel *output; /*!< Output channel - talking to bridge */ struct ast_channel *output; /*!< Output channel - talking to bridge */
}; };
@@ -93,28 +93,25 @@ static struct ast_frame *bridge_read(struct ast_channel *ast)
static int bridge_write(struct ast_channel *ast, struct ast_frame *f) static int bridge_write(struct ast_channel *ast, struct ast_frame *f)
{ {
struct bridge_pvt *p = ast->tech_pvt; struct bridge_pvt *p = ast->tech_pvt;
struct ast_channel *other; struct ast_channel *other = NULL;
ast_mutex_lock(&p->lock); ao2_lock(p);
/* only write frames to output. */
other = (p->input == ast ? p->output : p->input); if (p->input == ast) {
other = p->output;
while (other && ast_channel_trylock(other)) { if (other) {
ast_mutex_unlock(&p->lock); ast_channel_ref(other);
do { }
CHANNEL_DEADLOCK_AVOIDANCE(ast);
} while (ast_mutex_trylock(&p->lock));
other = (p->input == ast ? p->output : p->input);
} }
ao2_unlock(p);
/* We basically queue the frame up on the other channel if present */
if (other) { if (other) {
ast_channel_unlock(ast);
ast_queue_frame(other, f); ast_queue_frame(other, f);
ast_channel_unlock(other); ast_channel_lock(ast);
other = ast_channel_unref(other);
} }
ast_mutex_unlock(&p->lock);
return 0; return 0;
} }
@@ -129,64 +126,30 @@ static int bridge_call(struct ast_channel *ast, char *dest, int timeout)
} }
/* Impart the output channel upon the given bridge of the input channel */ /* Impart the output channel upon the given bridge of the input channel */
ast_bridge_impart(p->input->bridge, p->output, NULL, NULL); ast_bridge_impart(p->input->bridge, p->output, NULL, NULL, 0);
return 0; 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;
}
/*! \brief Called when a channel should be hung up */ /*! \brief Called when a channel should be hung up */
static int bridge_hangup(struct ast_channel *ast) static int bridge_hangup(struct ast_channel *ast)
{ {
struct bridge_pvt *p = ast->tech_pvt; struct bridge_pvt *p = ast->tech_pvt;
ast_mutex_lock(&p->lock); if (!p) {
return 0;
}
/* Figure out which channel this is... and set it to NULL as it has gone, but also queue up a hangup frame. */ ao2_lock(p);
if (p->input == ast) { if (p->input == ast) {
if (p->output) {
bridge_queue_hangup(p, ast);
}
p->input = NULL; p->input = NULL;
} else if (p->output == ast) { } else if (p->output == ast) {
if (p->input) {
bridge_queue_hangup(p, ast);
}
p->output = NULL; p->output = NULL;
} }
ao2_unlock(p);
/* Deal with the Asterisk portion of it */
ast->tech_pvt = NULL; ast->tech_pvt = NULL;
ao2_ref(p, -1);
/* 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);
}
return 0; return 0;
} }
@@ -198,26 +161,25 @@ static struct ast_channel *bridge_request(const char *type, struct ast_format_ca
struct ast_format slin; struct ast_format slin;
/* Try to allocate memory for our very minimal pvt structure */ /* 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; return NULL;
} }
/* Try to grab two Asterisk channels to use as input and output channels */ /* 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))) { 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; return NULL;
} }
if (!(p->output = ast_channel_alloc(1, AST_STATE_UP, 0, 0, "", "", "", requestor ? requestor->linkedid : NULL, 0, "Bridge/%p-output", p))) { 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); p->input = ast_channel_release(p->input);
ast_free(p); ao2_ref(p, -1);
return NULL; return NULL;
} }
/* Setup the lock on the pvt structure, we will need that */
ast_mutex_init(&p->lock);
/* Setup parameters on both new channels */ /* Setup parameters on both new channels */
p->input->tech = p->output->tech = &bridge_tech; p->input->tech = p->output->tech = &bridge_tech;
ao2_ref(p, 2);
p->input->tech_pvt = p->output->tech_pvt = p; p->input->tech_pvt = p->output->tech_pvt = p;
ast_format_set(&slin, AST_FORMAT_SLINEAR, 0); ast_format_set(&slin, AST_FORMAT_SLINEAR, 0);
@@ -236,6 +198,8 @@ static struct ast_channel *bridge_request(const char *type, struct ast_format_ca
ast_answer(p->output); ast_answer(p->output);
ast_answer(p->input); ast_answer(p->input);
/* remove the reference from the alloc. The channels now own the pvt. */
ao2_ref(p, -1);
return p->input; return p->input;
} }

7
include/asterisk/bridging.h
View File

@@ -156,6 +156,8 @@ struct ast_bridge_channel {
int fds[4]; int fds[4];
/*! Bit to indicate whether the channel is suspended from the bridge or not */ /*! Bit to indicate whether the channel is suspended from the bridge or not */
unsigned int suspended:1; 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 */ /*! Features structure for features that are specific to this channel */
struct ast_bridge_features *features; struct ast_bridge_features *features;
/*! Technology optimization parameters used by bridging technologies capable of /*! Technology optimization parameters used by bridging technologies capable of
@@ -339,6 +341,7 @@ enum ast_bridge_channel_state ast_bridge_join(struct ast_bridge *bridge,
* \param chan Channel to impart * \param chan Channel to impart
* \param swap Channel to swap out if swapping * \param swap Channel to swap out if swapping
* \param features Bridge features structure * \param features Bridge features structure
* \param allow_hangup Indicates if the bridge thread should manage hanging up of the channel or not.
* *
* \retval 0 on success * \retval 0 on success
* \retval -1 on failure * \retval -1 on failure
@@ -346,7 +349,7 @@ enum ast_bridge_channel_state ast_bridge_join(struct ast_bridge *bridge,
* Example usage: * Example usage:
* *
* \code * \code
* ast_bridge_impart(bridge, chan, NULL, NULL); * ast_bridge_impart(bridge, chan, NULL, NULL, 0);
* \endcode * \endcode
* *
* This adds a channel pointed to by the chan pointer to the bridge pointed to by * This adds a channel pointed to by the chan pointer to the bridge pointed to by
@@ -360,7 +363,7 @@ enum ast_bridge_channel_state ast_bridge_join(struct ast_bridge *bridge,
* If channel specific features are enabled a pointer to the features structure * If channel specific features are enabled a pointer to the features structure
* can be specified in the features parameter. * 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 /*! \brief Depart a channel from a bridge
* *

6
main/bridging.c
View File

@@ -1121,7 +1121,7 @@ static void *bridge_channel_thread(void *data)
state = bridge_channel_join(bridge_channel); 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 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); ast_hangup(bridge_channel->chan);
} }
@@ -1137,7 +1137,7 @@ static void *bridge_channel_thread(void *data)
return NULL; 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); struct ast_bridge_channel *bridge_channel = bridge_channel_alloc(bridge);
/* Try to allocate a structure for the bridge channel */ /* Try to allocate a structure for the bridge channel */
@@ -1149,6 +1149,8 @@ int ast_bridge_impart(struct ast_bridge *bridge, struct ast_channel *chan, struc
bridge_channel->chan = chan; bridge_channel->chan = chan;
bridge_channel->swap = swap; bridge_channel->swap = swap;
bridge_channel->features = features; bridge_channel->features = features;
bridge_channel->allow_impart_hangup = allow_hangup;
/* Actually create the thread that will handle the channel */ /* Actually create the thread that will handle the channel */
if (ast_pthread_create(&bridge_channel->thread, NULL, bridge_channel_thread, bridge_channel)) { if (ast_pthread_create(&bridge_channel->thread, NULL, bridge_channel_thread, bridge_channel)) {