bridge.c: Fixed race condition during attended transfer
authorKevin Harwell <kharwell@digium.com>
Wed, 8 Jul 2015 19:56:10 +0000 (14:56 -0500)
committerKevin Harwell <kharwell@digium.com>
Mon, 13 Jul 2015 17:57:56 +0000 (12:57 -0500)
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
include/asterisk/bridge_channel_internal.h
main/bridge.c
main/bridge_channel.c

index 8243a1d..8858cf0 100644 (file)
@@ -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.
index 71892f5..e3fb73d 100644 (file)
@@ -130,10 +130,47 @@ 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
index b2f7b0f..ebbfc39 100644 (file)
@@ -1549,7 +1549,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. */
@@ -1579,13 +1579,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
@@ -1606,14 +1607,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 */
@@ -1697,13 +1699,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) {
@@ -3953,6 +3969,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;
@@ -3983,8 +4008,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;
        }
 
@@ -3994,8 +4029,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);
index 1597226..28dfd9c 100644 (file)
@@ -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