[Asterisk-code-review] Attended transfers race condition (asterisk[13])
Kevin Harwell
asteriskteam at digium.com
Wed Jul 8 15:17:26 CDT 2015
Kevin Harwell has uploaded a new change for review.
https://gerrit.asterisk.org/839
Change subject: Attended transfers race condition
......................................................................
Attended transfers race condition
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
---
M include/asterisk/bridge.h
M include/asterisk/bridge_channel_internal.h
M main/bridge.c
M main/bridge_channel.c
4 files changed, 110 insertions(+), 12 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/39/839/1
diff --git a/include/asterisk/bridge.h b/include/asterisk/bridge.h
index 8fa0f36..c2247a8 100644
--- a/include/asterisk/bridge.h
+++ b/include/asterisk/bridge.h
@@ -509,6 +509,8 @@
* \param features Bridge features structure.
* \param flags defined by enum ast_bridge_impart_flags.
*
+ * \note This 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 71892f5..9e5080a 100644
--- a/include/asterisk/bridge_channel_internal.h
+++ b/include/asterisk/bridge_channel_internal.h
@@ -130,10 +130,48 @@
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;
+ /*! Thread's current result */
+ int res;
+ /*! The bridge channel */
+ struct ast_bridge_channel *bridge_channel;
+};
+
+/*!
+ * \internal
+ * \brief Wait for the expected signal.
+ *
+ * \param cond the wait object
+ *
+ * \retval the condition's result.
+ */
+int bridge_channel_internal_wait(struct bridge_channel_internal_cond *cond);
+
+/*!
+ * \internal
+ * \brief Set the result and signal the condition wait.
+ *
+ * \param cond the wait object
+ * \param res the result to use
+ *
+ * \retval the given res.
+ */
+int bridge_channel_internal_signal(struct bridge_channel_internal_cond *cond, int res);
+
+/*!
* \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 +186,8 @@
* \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 75cd671..120652e 100644
--- a/main/bridge.c
+++ b/main/bridge.c
@@ -1551,7 +1551,7 @@
}
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 @@
/*! \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 @@
/*! \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,28 @@
/* Actually create the thread that will handle the channel */
if (!res) {
+ struct bridge_channel_internal_cond cond = {
+ .done = 0,
+ .res = -1,
+ .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) {
+ res = bridge_channel_internal_wait(&cond);
+ }
+
+ ast_cond_destroy(&cond.cond);
+ ast_mutex_destroy(&cond.lock);
}
if (res) {
@@ -2341,6 +2358,7 @@
ast_answer(yanked_chan);
}
ast_channel_ref(yanked_chan);
+
if (ast_bridge_impart(bridge, yanked_chan, NULL, features,
AST_BRIDGE_IMPART_CHAN_INDEPENDENT)) {
/* It is possible for us to yank a channel and have some other
@@ -3985,19 +4003,35 @@
return AST_BRIDGE_TRANSFER_FAIL;
}
+ /*
+ * Since bridges need to be unlocked before entering ast_bridge_impart and
+ * core_local may call into it then bridge2 needs to be unlocked here
+ */
+ if (bridge2) {
+ ast_bridge_unlock(bridge2);
+ }
if (ast_call(local_chan, dest, 0)) {
ast_hangup(local_chan);
+ if (bridge2) {
+ ast_bridge_lock(bridge2);
+ }
return AST_BRIDGE_TRANSFER_FAIL;
+ }
+ if (bridge2) {
+ ast_bridge_lock(bridge2);
}
/* Get a ref for use later since this one is being stolen */
ao2_ref(local_chan, +1);
+ ast_bridge_unlock(bridge1);
if (ast_bridge_impart(bridge1, local_chan, chan1, NULL,
AST_BRIDGE_IMPART_CHAN_INDEPENDENT)) {
ast_hangup(local_chan);
ao2_cleanup(local_chan);
+ ast_bridge_lock(bridge1);
return AST_BRIDGE_TRANSFER_FAIL;
}
+ ast_bridge_lock(bridge1);
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 b7f0ba5..b534532 100644
--- a/main/bridge_channel.c
+++ b/main/bridge_channel.c
@@ -2560,7 +2560,30 @@
ao2_iterator_destroy(&iter);
}
-int bridge_channel_internal_join(struct ast_bridge_channel *bridge_channel)
+int 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);
+ return cond->res;
+}
+
+int bridge_channel_internal_signal(struct bridge_channel_internal_cond *cond, int res)
+{
+ if (cond && !cond->done) {
+ ast_mutex_lock(&cond->lock);
+ cond->res = res;
+ cond->done = 1;
+ ast_cond_signal(&cond->cond);
+ ast_mutex_unlock(&cond->lock);
+ }
+ return res;
+}
+
+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;
@@ -2569,13 +2592,11 @@
ast_debug(1, "Bridge %s: %p(%s) is joining\n",
bridge_channel->bridge->uniqueid,
bridge_channel, ast_channel_name(bridge_channel->chan));
-
/*
* Directly locking the bridge is safe here because nobody else
* knows about this bridge_channel yet.
*/
ast_bridge_lock(bridge_channel->bridge);
-
ast_channel_lock(bridge_channel->chan);
bridge_channel->read_format = ao2_bump(ast_channel_readformat(bridge_channel->chan));
@@ -2590,7 +2611,7 @@
bridge_channel->bridge->uniqueid,
bridge_channel,
ast_channel_name(bridge_channel->chan));
- return -1;
+ return bridge_channel_internal_signal(cond, -1);
}
ast_channel_internal_bridge_set(bridge_channel->chan, bridge_channel->bridge);
@@ -2642,6 +2663,7 @@
swap = NULL;
bridge_channel_event_join_leave(bridge_channel, AST_BRIDGE_HOOK_TYPE_JOIN);
+ bridge_channel_internal_signal(cond, 0);
while (bridge_channel->state == BRIDGE_CHANNEL_STATE_WAIT) {
/* Wait for something to do. */
@@ -2660,6 +2682,7 @@
bridge_reconfigured(bridge_channel->bridge, 1);
ast_bridge_unlock(bridge_channel->bridge);
+ bridge_channel_internal_signal(cond, 0);
/* Must release any swap ref after unlocking the bridge. */
ao2_t_cleanup(swap, "Bridge push with swap failed or exited immediately");
--
To view, visit https://gerrit.asterisk.org/839
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I08fe33a2560da924e676df55b181e46fca604577
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
More information about the asterisk-code-review
mailing list