From 97ee0ee6c6d5f37591183339999d8cb936bf517a Mon Sep 17 00:00:00 2001 From: Kevin Harwell Date: Wed, 8 Jul 2015 14:56:10 -0500 Subject: [PATCH] bridge.c: Fixed race condition during attended transfer During an attended transfer a thread is started that handles imparting the bridge channel. From the start of the thread to when the bridge channel is ready exists a gap that can potentially cause problems (for instance, the channel being swapped is hung up before the replacement channel enters the bridge thus stopping the transfer). This patch adds a condition that waits for the impart thread to get to a point of acceptable readiness before allowing the initiating thread to continue. ASTERISK-24782 Reported by: John Bigelow Change-Id: I08fe33a2560da924e676df55b181e46fca604577 --- include/asterisk/bridge.h | 2 + include/asterisk/bridge_channel_internal.h | 40 ++++++++++++++++- main/bridge.c | 51 +++++++++++++++++++--- main/bridge_channel.c | 25 ++++++++++- 4 files changed, 109 insertions(+), 9 deletions(-) diff --git a/include/asterisk/bridge.h b/include/asterisk/bridge.h index 8fa0f36ede..fc3726270f 100644 --- a/include/asterisk/bridge.h +++ b/include/asterisk/bridge.h @@ -509,6 +509,8 @@ enum ast_bridge_impart_flags { * \param features Bridge features structure. * \param flags defined by enum ast_bridge_impart_flags. * + * \note The given bridge must be unlocked when calling this function. + * * \note The features parameter must be NULL or obtained by * ast_bridge_features_new(). You must not dereference features * after calling even if the call fails. diff --git a/include/asterisk/bridge_channel_internal.h b/include/asterisk/bridge_channel_internal.h index 71892f51a0..e3fb73d7e1 100644 --- a/include/asterisk/bridge_channel_internal.h +++ b/include/asterisk/bridge_channel_internal.h @@ -129,11 +129,48 @@ int bridge_channel_internal_push(struct ast_bridge_channel *bridge_channel); */ void bridge_channel_internal_pull(struct ast_bridge_channel *bridge_channel); +/*! + * \brief Internal bridge channel wait condition and associated result. + */ +struct bridge_channel_internal_cond { + /*! Lock for the data structure */ + ast_mutex_t lock; + /*! Wait condition */ + ast_cond_t cond; + /*! Wait until done */ + int done; + /*! The bridge channel */ + struct ast_bridge_channel *bridge_channel; +}; + +/*! + * \internal + * \brief Wait for the expected signal. + * \since 13.5.0 + * + * \param cond the wait object + * + * \return Nothing + */ +void bridge_channel_internal_wait(struct bridge_channel_internal_cond *cond); + +/*! + * \internal + * \brief Signal the condition wait. + * \since 13.5.0 + * + * \param cond the wait object + * + * \return Nothing + */ +void bridge_channel_internal_signal(struct bridge_channel_internal_cond *cond); + /*! * \internal * \brief Join the bridge_channel to the bridge (blocking) * * \param bridge_channel The Channel in the bridge + * \param cond data used for signaling * * \note The bridge_channel->swap holds a channel reference for the swap * channel going into the bridging system. The ref ensures that the swap @@ -148,7 +185,8 @@ void bridge_channel_internal_pull(struct ast_bridge_channel *bridge_channel); * \retval 0 bridge channel successfully joined the bridge * \retval -1 bridge channel failed to join the bridge */ -int bridge_channel_internal_join(struct ast_bridge_channel *bridge_channel); +int bridge_channel_internal_join(struct ast_bridge_channel *bridge_channel, + struct bridge_channel_internal_cond *cond); /*! * \internal diff --git a/main/bridge.c b/main/bridge.c index 75cd671bfe..7644884ebe 100644 --- a/main/bridge.c +++ b/main/bridge.c @@ -1551,7 +1551,7 @@ int ast_bridge_join(struct ast_bridge *bridge, } if (!res) { - res = bridge_channel_internal_join(bridge_channel); + res = bridge_channel_internal_join(bridge_channel, NULL); } /* Cleanup all the data in the bridge channel after it leaves the bridge. */ @@ -1581,13 +1581,14 @@ join_exit:; /*! \brief Thread responsible for imparted bridged channels to be departed */ static void *bridge_channel_depart_thread(void *data) { - struct ast_bridge_channel *bridge_channel = data; + struct bridge_channel_internal_cond *cond = data; + struct ast_bridge_channel *bridge_channel = cond->bridge_channel; if (bridge_channel->callid) { ast_callid_threadassoc_add(bridge_channel->callid); } - bridge_channel_internal_join(bridge_channel); + bridge_channel_internal_join(bridge_channel, cond); /* * cleanup @@ -1608,14 +1609,15 @@ static void *bridge_channel_depart_thread(void *data) /*! \brief Thread responsible for independent imparted bridged channels */ static void *bridge_channel_ind_thread(void *data) { - struct ast_bridge_channel *bridge_channel = data; + struct bridge_channel_internal_cond *cond = data; + struct ast_bridge_channel *bridge_channel = cond->bridge_channel; struct ast_channel *chan; if (bridge_channel->callid) { ast_callid_threadassoc_add(bridge_channel->callid); } - bridge_channel_internal_join(bridge_channel); + bridge_channel_internal_join(bridge_channel, cond); chan = bridge_channel->chan; /* cleanup */ @@ -1699,13 +1701,27 @@ int ast_bridge_impart(struct ast_bridge *bridge, /* Actually create the thread that will handle the channel */ if (!res) { + struct bridge_channel_internal_cond cond = { + .done = 0, + .bridge_channel = bridge_channel + }; + ast_mutex_init(&cond.lock); + ast_cond_init(&cond.cond, NULL); + if ((flags & AST_BRIDGE_IMPART_CHAN_MASK) == AST_BRIDGE_IMPART_CHAN_INDEPENDENT) { res = ast_pthread_create_detached(&bridge_channel->thread, NULL, - bridge_channel_ind_thread, bridge_channel); + bridge_channel_ind_thread, &cond); } else { res = ast_pthread_create(&bridge_channel->thread, NULL, - bridge_channel_depart_thread, bridge_channel); + bridge_channel_depart_thread, &cond); } + + if (!res) { + bridge_channel_internal_wait(&cond); + } + + ast_cond_destroy(&cond.cond); + ast_mutex_destroy(&cond.lock); } if (res) { @@ -3955,6 +3971,15 @@ static enum ast_transfer_result attended_transfer_bridge(struct ast_channel *cha struct ast_channel *chan2, struct ast_bridge *bridge1, struct ast_bridge *bridge2, struct ast_attended_transfer_message *transfer_msg) { +#define BRIDGE_LOCK_ONE_OR_BOTH(b1, b2) \ + do { \ + if (b2) { \ + ast_bridge_lock_both(b1, b2); \ + } else { \ + ast_bridge_lock(b1); \ + } \ + } while (0) + static const char *dest = "_attended@transfer/m"; struct ast_channel *local_chan; int cause; @@ -3985,8 +4010,18 @@ static enum ast_transfer_result attended_transfer_bridge(struct ast_channel *cha return AST_BRIDGE_TRANSFER_FAIL; } + /* + * Since bridges need to be unlocked before entering ast_bridge_impart and + * core_local may call into it then the bridges need to be unlocked here. + */ + ast_bridge_unlock(bridge1); + if (bridge2) { + ast_bridge_unlock(bridge2); + } + if (ast_call(local_chan, dest, 0)) { ast_hangup(local_chan); + BRIDGE_LOCK_ONE_OR_BOTH(bridge1, bridge2); return AST_BRIDGE_TRANSFER_FAIL; } @@ -3996,8 +4031,10 @@ static enum ast_transfer_result attended_transfer_bridge(struct ast_channel *cha AST_BRIDGE_IMPART_CHAN_INDEPENDENT)) { ast_hangup(local_chan); ao2_cleanup(local_chan); + BRIDGE_LOCK_ONE_OR_BOTH(bridge1, bridge2); return AST_BRIDGE_TRANSFER_FAIL; } + BRIDGE_LOCK_ONE_OR_BOTH(bridge1, bridge2); if (bridge2) { RAII_VAR(struct ast_channel *, local_chan2, NULL, ao2_cleanup); diff --git a/main/bridge_channel.c b/main/bridge_channel.c index b7f0ba51ab..3ee04db55d 100644 --- a/main/bridge_channel.c +++ b/main/bridge_channel.c @@ -2560,7 +2560,27 @@ static void bridge_channel_event_join_leave(struct ast_bridge_channel *bridge_ch ao2_iterator_destroy(&iter); } -int bridge_channel_internal_join(struct ast_bridge_channel *bridge_channel) +void bridge_channel_internal_wait(struct bridge_channel_internal_cond *cond) +{ + ast_mutex_lock(&cond->lock); + while (!cond->done) { + ast_cond_wait(&cond->cond, &cond->lock); + } + ast_mutex_unlock(&cond->lock); +} + +void bridge_channel_internal_signal(struct bridge_channel_internal_cond *cond) +{ + if (cond) { + ast_mutex_lock(&cond->lock); + cond->done = 1; + ast_cond_signal(&cond->cond); + ast_mutex_unlock(&cond->lock); + } +} + +int bridge_channel_internal_join(struct ast_bridge_channel *bridge_channel, + struct bridge_channel_internal_cond *cond) { int res = 0; struct ast_bridge_features *channel_features; @@ -2590,6 +2610,7 @@ int bridge_channel_internal_join(struct ast_bridge_channel *bridge_channel) bridge_channel->bridge->uniqueid, bridge_channel, ast_channel_name(bridge_channel->chan)); + bridge_channel_internal_signal(cond); return -1; } ast_channel_internal_bridge_set(bridge_channel->chan, bridge_channel->bridge); @@ -2624,6 +2645,8 @@ int bridge_channel_internal_join(struct ast_bridge_channel *bridge_channel) } bridge_reconfigured(bridge_channel->bridge, !bridge_channel->inhibit_colp); + bridge_channel_internal_signal(cond); + if (bridge_channel->state == BRIDGE_CHANNEL_STATE_WAIT) { /* * Indicate a source change since this channel is entering the