From 97ee0ee6c6d5f37591183339999d8cb936bf517a Mon Sep 17 00:00:00 2001 From: Kevin Harwell Date: Wed, 8 Jul 2015 14:56:10 -0500 Subject: 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 --- main/bridge.c | 51 ++++++++++++++++++++++++++++++++++++++++++++------- main/bridge_channel.c | 25 ++++++++++++++++++++++++- 2 files changed, 68 insertions(+), 8 deletions(-) (limited to 'main') diff --git a/main/bridge.c b/main/bridge.c index 75cd671bf..7644884eb 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 b7f0ba51a..3ee04db55 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 -- cgit v1.2.3