From a8276fe8efbca7e14862c469c0c95df71b506947 Mon Sep 17 00:00:00 2001 From: Matthew Jordan Date: Fri, 13 Jan 2012 16:48:06 +0000 Subject: 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 git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@350551 65c4cc65-6c06-0410-ace0-fbb531ad65f3 --- apps/app_confbridge.c | 6 ++- bridges/bridge_builtin_features.c | 8 ++-- channels/chan_bridge.c | 90 ++++++++++++--------------------------- include/asterisk/bridging.h | 7 ++- main/bridging.c | 6 ++- 5 files changed, 44 insertions(+), 73 deletions(-) diff --git a/apps/app_confbridge.c b/apps/app_confbridge.c index 8683c95f8..1103b6e9e 100644 --- a/apps/app_confbridge.c +++ b/apps/app_confbridge.c @@ -874,7 +874,9 @@ static void destroy_conference_bridge(void *obj) 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 @@ static int play_sound_helper(struct conference_bridge *conference_bridge, const } 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 */ diff --git a/bridges/bridge_builtin_features.c b/bridges/bridge_builtin_features.c index 5fdb664d5..9da02f1aa 100644 --- a/bridges/bridge_builtin_features.c +++ b/bridges/bridge_builtin_features.c @@ -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. */ - 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 @@ static int feature_attended_transfer(struct ast_bridge *bridge, struct ast_bridg 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 @@ 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 (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); diff --git a/channels/chan_bridge.c b/channels/chan_bridge.c index 36f23a500..3e12b3995 100644 --- a/channels/chan_bridge.c +++ b/channels/chan_bridge.c @@ -49,6 +49,7 @@ ASTERISK_FILE_VERSION(__FILE__, "$Revision$") #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 @@ static struct ast_channel_tech bridge_tech = { }; 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,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) { struct bridge_pvt *p = ast->tech_pvt; - struct ast_channel *other; + struct ast_channel *other = NULL; - 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); + ao2_lock(p); + /* only write frames to output. */ + if (p->input == ast) { + other = p->output; + if (other) { + ast_channel_ref(other); + } } + ao2_unlock(p); - /* We basically queue the frame up on the other channel if present */ if (other) { + ast_channel_unlock(ast); 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; } @@ -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 */ - ast_bridge_impart(p->input->bridge, p->output, NULL, NULL); + ast_bridge_impart(p->input->bridge, p->output, NULL, NULL, 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 */ static int bridge_hangup(struct ast_channel *ast) { 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->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; } + ao2_unlock(p); - /* Deal with the Asterisk portion of it */ 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 @@ static struct ast_channel *bridge_request(const char *type, struct ast_format_ca 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 @@ static struct ast_channel *bridge_request(const char *type, struct ast_format_ca 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; } diff --git a/include/asterisk/bridging.h b/include/asterisk/bridging.h index 54137b028..3c117b7cd 100644 --- a/include/asterisk/bridging.h +++ b/include/asterisk/bridging.h @@ -156,6 +156,8 @@ struct ast_bridge_channel { 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,6 +341,7 @@ enum ast_bridge_channel_state ast_bridge_join(struct ast_bridge *bridge, * \param chan Channel to impart * \param swap Channel to swap out if swapping * \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 -1 on failure @@ -346,7 +349,7 @@ enum ast_bridge_channel_state ast_bridge_join(struct ast_bridge *bridge, * Example usage: * * \code - * ast_bridge_impart(bridge, chan, NULL, NULL); + * 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 @@ enum ast_bridge_channel_state ast_bridge_join(struct ast_bridge *bridge, * 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 * diff --git a/main/bridging.c b/main/bridging.c index abf5a0386..b97150f04 100644 --- a/main/bridging.c +++ b/main/bridging.c @@ -1121,7 +1121,7 @@ static void *bridge_channel_thread(void *data) 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 @@ static void *bridge_channel_thread(void *data) 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 @@ int ast_bridge_impart(struct ast_bridge *bridge, struct ast_channel *chan, struc 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)) { -- cgit v1.2.3