Bridge core: Pass a ref with the swap channel when joining a bridge.
authorRichard Mudgett <rmudgett@digium.com>
Thu, 22 Jan 2015 19:30:12 +0000 (19:30 +0000)
committerRichard Mudgett <rmudgett@digium.com>
Thu, 22 Jan 2015 19:30:12 +0000 (19:30 +0000)
When code imparts a channel into a bridge to swap with another channel, a
ref needs to be held on the swap channel to ensure that it cannot
dissapear before finding it in the bridge.

* The ast_bridge_join() swap channel parameter now always steals a ref for
the swap channel.  This is the only change to the bridge framework's
public API semantics.

* bridge_channel_internal_join() now requires the bridge_channel->swap
channel to pass in a ref.

ASTERISK-24649
Reported by: John Bigelow

Review: https://reviewboard.asterisk.org/r/4354/
........

Merged revisions 430975 from http://svn.asterisk.org/svn/asterisk/branches/13

git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@430976 65c4cc65-6c06-0410-ace0-fbb531ad65f3

include/asterisk/bridge.h
include/asterisk/bridge_channel_internal.h
include/asterisk/bridge_internal.h
main/bridge.c
main/bridge_channel.c

index ccfb541..fecca1d 100644 (file)
@@ -444,15 +444,19 @@ enum ast_bridge_join_flags {
 };
 
 /*!
- * \brief Join (blocking) a channel to a bridge
+ * \brief Join a channel to a bridge (blocking)
  *
  * \param bridge Bridge to join
  * \param chan Channel to join
- * \param swap Channel to swap out if swapping
+ * \param swap Channel to swap out if swapping (A channel reference is stolen.)
  * \param features Bridge features structure
  * \param tech_args Optional Bridging tech optimization parameters for this channel.
  * \param flags defined by enum ast_bridge_join_flags.
  *
+ * \note The passed in swap channel is always unreffed on return.  It is not a
+ * good idea to access the swap channel on return or for the caller to keep a
+ * reference to it.
+ *
  * \note Absolutely _NO_ locks should be held before calling
  * this function since it blocks.
  *
@@ -495,7 +499,7 @@ enum ast_bridge_impart_flags {
 };
 
 /*!
- * \brief Impart (non-blocking) a channel onto a bridge
+ * \brief Impart a channel to a bridge (non-blocking)
  *
  * \param bridge Bridge to impart on
  * \param chan Channel to impart (The channel reference is stolen if impart successful.)
index b5a3a8d..71892f5 100644 (file)
@@ -103,6 +103,9 @@ void bridge_channel_settle_owed_events(struct ast_bridge *orig_bridge, struct as
  *
  * \param bridge_channel Channel to push.
  *
+ * \note A ref is not held by bridge_channel->swap when calling because the
+ * push with swap happens immediately.
+ *
  * \note On entry, bridge_channel->bridge is already locked.
  *
  * \retval 0 on success.
@@ -128,16 +131,22 @@ void bridge_channel_internal_pull(struct ast_bridge_channel *bridge_channel);
 
 /*!
  * \internal
- * \brief Join the bridge_channel to the bridge
+ * \brief Join the bridge_channel to the bridge (blocking)
  *
  * \param bridge_channel The Channel in the bridge
  *
+ * \note The bridge_channel->swap holds a channel reference for the swap
+ * channel going into the bridging system.  The ref ensures that the swap
+ * pointer is valid for the bridge subclass push callbacks.  The pointer
+ * will be NULL on return if the ref was consumed.
+ *
+ * \details
+ * This API call puts the bridge_channel into the bridge and handles the
+ * bridge_channel's processing of events while it is in the bridge.  It
+ * will return when the channel has been instructed to leave the bridge.
+ *
  * \retval 0 bridge channel successfully joined the bridge
  * \retval -1 bridge channel failed to join the bridge
- *
- * \note This API call starts the bridge_channel's processing of events while
- * it is in the bridge. It will return when the channel has been instructed to
- * leave the bridge.
  */
 int bridge_channel_internal_join(struct ast_bridge_channel *bridge_channel);
 
index e50e7f9..37c8418 100644 (file)
@@ -117,6 +117,9 @@ struct ast_bridge *bridge_base_init(struct ast_bridge *self, uint32_t capabiliti
  * \param attempt_recovery TRUE if failure attempts to push channel back into original bridge.
  * \param optimized Indicates whether the move is part of an unreal channel optimization.
  *
+ * \note A ref is not held by bridge_channel->swap when calling because the
+ * move with swap happens immediately.
+ *
  * \note The dst_bridge and bridge_channel->bridge are assumed already locked.
  *
  * \retval 0 on success.
index 6b4778f..9fb6e32 100644 (file)
@@ -1560,6 +1560,7 @@ int ast_bridge_join(struct ast_bridge *bridge,
                ao2_ref(bridge, -1);
        }
        if (!bridge_channel) {
+               ao2_t_cleanup(swap, "Error exit: bridge_channel alloc failed");
                res = -1;
                goto join_exit;
        }
@@ -1567,6 +1568,7 @@ int ast_bridge_join(struct ast_bridge *bridge,
        ast_assert(features != NULL);
        if (!features) {
                ao2_ref(bridge_channel, -1);
+               ao2_t_cleanup(swap, "Error exit: features is NULL");
                res = -1;
                goto join_exit;
        }
@@ -1596,6 +1598,8 @@ int ast_bridge_join(struct ast_bridge *bridge,
        ast_channel_internal_bridge_channel_set(chan, NULL);
        ast_channel_unlock(chan);
        bridge_channel->chan = NULL;
+       /* If bridge_channel->swap is not NULL then the join failed. */
+       ao2_t_cleanup(bridge_channel->swap, "Bridge complete: join failed");
        bridge_channel->swap = NULL;
        bridge_channel->features = NULL;
 
@@ -1624,7 +1628,12 @@ static void *bridge_channel_depart_thread(void *data)
 
        bridge_channel_internal_join(bridge_channel);
 
-       /* cleanup */
+       /*
+        * cleanup
+        *
+        * If bridge_channel->swap is not NULL then the join failed.
+        */
+       ao2_t_cleanup(bridge_channel->swap, "Bridge complete: Departable impart join failed");
        bridge_channel->swap = NULL;
        ast_bridge_features_destroy(bridge_channel->features);
        bridge_channel->features = NULL;
@@ -1653,6 +1662,8 @@ static void *bridge_channel_ind_thread(void *data)
        ast_channel_internal_bridge_channel_set(chan, NULL);
        ast_channel_unlock(chan);
        bridge_channel->chan = NULL;
+       /* If bridge_channel->swap is not NULL then the join failed. */
+       ao2_t_cleanup(bridge_channel->swap, "Bridge complete: Independent impart join failed");
        bridge_channel->swap = NULL;
        ast_bridge_features_destroy(bridge_channel->features);
        bridge_channel->features = NULL;
@@ -1706,7 +1717,7 @@ int ast_bridge_impart(struct ast_bridge *bridge,
        }
        ast_channel_unlock(chan);
        bridge_channel->chan = chan;
-       bridge_channel->swap = swap;
+       bridge_channel->swap = ao2_t_bump(swap, "Setting up bridge impart");
        bridge_channel->features = features;
        bridge_channel->inhibit_colp = !!(flags & AST_BRIDGE_IMPART_INHIBIT_JOIN_COLP);
        bridge_channel->depart_wait =
@@ -1730,6 +1741,7 @@ int ast_bridge_impart(struct ast_bridge *bridge,
                ast_channel_internal_bridge_channel_set(chan, NULL);
                ast_channel_unlock(chan);
                bridge_channel->chan = NULL;
+               ao2_t_cleanup(bridge_channel->swap, "Bridge complete: Impart failed");
                bridge_channel->swap = NULL;
                ast_bridge_features_destroy(bridge_channel->features);
                bridge_channel->features = NULL;
@@ -2171,7 +2183,11 @@ int bridge_do_move(struct ast_bridge *dst_bridge, struct ast_bridge_channel *bri
                /*
                 * The channel died as a result of being pulled.  Leave it
                 * pointing to the original bridge.
+                *
+                * Clear out the swap channel pointer.  A ref is not held
+                * by bridge_channel->swap at this point.
                 */
+               bridge_channel->swap = NULL;
                bridge_reconfigured(orig_bridge, 0);
                return -1;
        }
index e59d28e..ff72c57 100644 (file)
@@ -2490,11 +2490,11 @@ static void bridge_channel_event_join_leave(struct ast_bridge_channel *bridge_ch
        ao2_iterator_destroy(&iter);
 }
 
-/*! \brief Join a channel to a bridge and handle anything the bridge may want us to do */
 int bridge_channel_internal_join(struct ast_bridge_channel *bridge_channel)
 {
        int res = 0;
        struct ast_bridge_features *channel_features;
+       struct ast_channel *swap;
 
        ast_debug(1, "Bridge %s: %p(%s) is joining\n",
                bridge_channel->bridge->uniqueid,
@@ -2538,6 +2538,9 @@ int bridge_channel_internal_join(struct ast_bridge_channel *bridge_channel)
                bridge_channel->bridge->callid = ast_read_threadstorage_callid();
        }
 
+       /* Take the swap channel ref from the bridge_channel struct. */
+       swap = bridge_channel->swap;
+
        if (bridge_channel_internal_push(bridge_channel)) {
                int cause = bridge_channel->bridge->cause;
 
@@ -2563,6 +2566,11 @@ int bridge_channel_internal_join(struct ast_bridge_channel *bridge_channel)
                }
 
                ast_bridge_unlock(bridge_channel->bridge);
+
+               /* Must release any swap ref after unlocking the bridge. */
+               ao2_t_cleanup(swap, "Bridge push with swap successful");
+               swap = NULL;
+
                bridge_channel_event_join_leave(bridge_channel, AST_BRIDGE_HOOK_TYPE_JOIN);
 
                while (bridge_channel->state == BRIDGE_CHANNEL_STATE_WAIT) {
@@ -2583,6 +2591,9 @@ int bridge_channel_internal_join(struct ast_bridge_channel *bridge_channel)
 
        ast_bridge_unlock(bridge_channel->bridge);
 
+       /* Must release any swap ref after unlocking the bridge. */
+       ao2_t_cleanup(swap, "Bridge push with swap failed or exited immediately");
+
        /* Complete any active hold before exiting the bridge. */
        if (ast_channel_hold_state(bridge_channel->chan) == AST_CONTROL_HOLD) {
                ast_debug(1, "Channel %s simulating UNHOLD for bridge end.\n",