bridging: Ensure locking during snapshot creation

While the vast majority of bridge snapshot creation is locked properly,
there are currently some instances that are not. This adds the missing
locking to ensure bridge state is not malleable during snapshot
creation.

(closes issue ASTERISK-22904)
Review: https://reviewboard.asterisk.org/r/3415/
Reported by: Matt Jordan
........

Merged revisions 412193 from http://svn.asterisk.org/svn/asterisk/branches/12

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

diff --git a/apps/app_confbridge.c b/apps/app_confbridge.c
index b034ab3..3a20101 100644 (file)
--- a/apps/app_confbridge.c
+++ b/apps/app_confbridge.c
@@ -453,10 +453,13 @@ static void send_conf_stasis(struct confbridge_conference *conference, struct as
                ast_json_object_update(json_object, extras);
        }
 
                ast_json_object_update(json_object, extras);
        }
 

+       ast_bridge_lock(conference->bridge);

        msg = ast_bridge_blob_create(type,
                conference->bridge,
                chan,
                json_object);
        msg = ast_bridge_blob_create(type,
                conference->bridge,
                chan,
                json_object);

+       ast_bridge_unlock(conference->bridge);
+

        if (!msg) {
                return;
        }
        if (!msg) {
                return;
        }

diff --git a/include/asterisk/stasis_bridges.h b/include/asterisk/stasis_bridges.h
index 72bf599..49d4db2 100644 (file)
--- a/include/asterisk/stasis_bridges.h
+++ b/include/asterisk/stasis_bridges.h
@@ -65,6 +65,8 @@ struct ast_bridge_snapshot {
  * \brief Generate a snapshot of the bridge state. This is an ao2 object, so
  * ao2_cleanup() to deallocate.
  *
  * \brief Generate a snapshot of the bridge state. This is an ao2 object, so
  * ao2_cleanup() to deallocate.
  *

+ * \pre Bridge is locked
+ *

  * \param bridge The bridge from which to generate a snapshot
  *
  * \retval AO2 refcounted snapshot on success
  * \param bridge The bridge from which to generate a snapshot
  *
  * \retval AO2 refcounted snapshot on success

@@ -136,6 +138,8 @@ struct stasis_cache *ast_bridge_cache(void);
  * \since 12
  * \brief Publish the state of a bridge
  *
  * \since 12
  * \brief Publish the state of a bridge
  *

+ * \pre Bridge is locked
+ *

  * \param bridge The bridge for which to publish state
  */
 void ast_bridge_publish_state(struct ast_bridge *bridge);
  * \param bridge The bridge for which to publish state
  */
 void ast_bridge_publish_state(struct ast_bridge *bridge);

@@ -158,6 +162,8 @@ struct stasis_message_type *ast_bridge_merge_message_type(void);
  * \since 12
  * \brief Publish a bridge merge
  *
  * \since 12
  * \brief Publish a bridge merge
  *

+ * \pre Bridges involved are locked
+ *

  * \param to The bridge to which channels are being added
  * \param from The bridge from which channels are being removed
  */
  * \param to The bridge to which channels are being added
  * \param from The bridge from which channels are being removed
  */

@@ -281,7 +287,7 @@ struct stasis_message_type *ast_blind_transfer_type(void);
 /*!
  * \brief Publish a blind transfer event
  *
 /*!
  * \brief Publish a blind transfer event
  *

- * \pre No channels or bridges are locked
+ * \pre Bridges involved are locked. Channels involved are not locked.

  *
  * \param is_external Whether the blind transfer was initiated externally (e.g. via AMI or native protocol)
  * \param result The success or failure of the transfer
  *
  * \param is_external Whether the blind transfer was initiated externally (e.g. via AMI or native protocol)
  * \param result The success or failure of the transfer

@@ -346,7 +352,7 @@ struct stasis_message_type *ast_attended_transfer_type(void);
  * Publish an \ref ast_attended_transfer_message with the dest_type set to
  * \c AST_ATTENDED_TRANSFER_DEST_FAIL.
  *
  * Publish an \ref ast_attended_transfer_message with the dest_type set to
  * \c AST_ATTENDED_TRANSFER_DEST_FAIL.
  *

- * \pre No channels or bridges are locked
+ * \pre Bridges involved are locked. Channels involved are not locked.

  *
  * \param is_external Indicates if the transfer was initiated externally
  * \param result The result of the transfer. Will always be a type of failure.
  *
  * \param is_external Indicates if the transfer was initiated externally
  * \param result The result of the transfer. Will always be a type of failure.

@@ -369,7 +375,7 @@ void ast_bridge_publish_attended_transfer_fail(int is_external, enum ast_transfe
  *
  * In either case, two bridges enter, one leaves.
  *
  *
  * In either case, two bridges enter, one leaves.
  *

- * \pre No channels or bridges are locked
+ * \pre Bridges involved are locked. Channels involved are not locked.

  *
  * \param is_external Indicates if the transfer was initiated externally
  * \param result The result of the transfer.
  *
  * \param is_external Indicates if the transfer was initiated externally
  * \param result The result of the transfer.

@@ -390,7 +396,7 @@ void ast_bridge_publish_attended_transfer_bridge_merge(int is_external, enum ast
  * this results from merging two bridges together. The difference is that a
  * transferer channel survives the bridge merge
  *
  * this results from merging two bridges together. The difference is that a
  * transferer channel survives the bridge merge
  *

- * \pre No channels or bridges are locked
+ * \pre Bridges involved are locked. Channels involved are not locked.

  *
  * \param is_external Indicates if the transfer was initiated externally
  * \param result The result of the transfer.
  *
  * \param is_external Indicates if the transfer was initiated externally
  * \param result The result of the transfer.

@@ -413,7 +419,7 @@ void ast_bridge_publish_attended_transfer_threeway(int is_external, enum ast_tra
  * \li A transferee channel leaving a bridge to run an app
  * \li A bridge of transferees running an app (via a local channel)
  *
  * \li A transferee channel leaving a bridge to run an app
  * \li A bridge of transferees running an app (via a local channel)
  *

- * \pre No channels or bridges are locked
+ * \pre Bridges involved are locked. Channels involved are not locked.

  *
  * \param is_external Indicates if the transfer was initiated externally
  * \param result The result of the transfer.
  *
  * \param is_external Indicates if the transfer was initiated externally
  * \param result The result of the transfer.

@@ -438,7 +444,7 @@ void ast_bridge_publish_attended_transfer_app(int is_external, enum ast_transfer
  * When this type of transfer occurs, the two bridges continue to exist after the
  * transfer and a local channel is used to link the two bridges together.
  *
  * When this type of transfer occurs, the two bridges continue to exist after the
  * transfer and a local channel is used to link the two bridges together.
  *

- * \pre No channels or bridges are locked
+ * \pre Bridges involved are locked. Channels involved are not locked.

  *
  * \param is_external Indicates if the transfer was initiated externally
  * \param result The result of the transfer.
  *
  * \param is_external Indicates if the transfer was initiated externally
  * \param result The result of the transfer.

diff --git a/main/bridge.c b/main/bridge.c
index 1baf01c..ee1e4a8 100644 (file)
--- a/main/bridge.c
+++ b/main/bridge.c
@@ -615,7 +615,10 @@ static struct stasis_message *create_bridge_snapshot_message(struct ast_bridge *
 {
        RAII_VAR(struct ast_bridge_snapshot *, snapshot, NULL, ao2_cleanup);
 
 {
        RAII_VAR(struct ast_bridge_snapshot *, snapshot, NULL, ao2_cleanup);
 

+       ast_bridge_lock(bridge);

        snapshot = ast_bridge_snapshot_create(bridge);
        snapshot = ast_bridge_snapshot_create(bridge);

+       ast_bridge_unlock(bridge);
+

        if (!snapshot) {
                return NULL;
        }
        if (!snapshot) {
                return NULL;
        }

@@ -689,7 +692,9 @@ struct ast_bridge *bridge_register(struct ast_bridge *bridge)
 {
        if (bridge) {
                bridge->construction_completed = 1;
 {
        if (bridge) {
                bridge->construction_completed = 1;

+               ast_bridge_lock(bridge);

                ast_bridge_publish_state(bridge);
                ast_bridge_publish_state(bridge);

+               ast_bridge_unlock(bridge);

                if (!ao2_link(bridges, bridge)) {
                        ast_bridge_destroy(bridge, 0);
                        bridge = NULL;
                if (!ao2_link(bridges, bridge)) {
                        ast_bridge_destroy(bridge, 0);
                        bridge = NULL;

@@ -4067,7 +4072,9 @@ static void publish_blind_transfer(int is_external, enum ast_transfer_result res
        struct ast_bridge_channel_pair pair;
        pair.channel = transferer;
        pair.bridge = bridge;
        struct ast_bridge_channel_pair pair;
        pair.channel = transferer;
        pair.bridge = bridge;

+       ast_bridge_lock(bridge);

        ast_bridge_publish_blind_transfer(is_external, result, &pair, context, exten);
        ast_bridge_publish_blind_transfer(is_external, result, &pair, context, exten);

+       ast_bridge_unlock(bridge);

 }
 
 enum ast_transfer_result ast_bridge_transfer_blind(int is_external,
 }
 
 enum ast_transfer_result ast_bridge_transfer_blind(int is_external,

@@ -4294,7 +4301,7 @@ enum ast_transfer_result ast_bridge_transfer_attended(struct ast_channel *to_tra
        RAII_VAR(struct ast_bridge_channel *, to_target_bridge_channel, NULL, ao2_cleanup);
        RAII_VAR(struct ao2_container *, channels, NULL, ao2_cleanup);
        RAII_VAR(struct ast_channel *, transferee, NULL, ao2_cleanup);
        RAII_VAR(struct ast_bridge_channel *, to_target_bridge_channel, NULL, ao2_cleanup);
        RAII_VAR(struct ao2_container *, channels, NULL, ao2_cleanup);
        RAII_VAR(struct ast_channel *, transferee, NULL, ao2_cleanup);

-       struct ast_bridge *the_bridge;
+       struct ast_bridge *the_bridge = NULL;

        struct ast_channel *chan_bridged;
        struct ast_channel *chan_unbridged;
        int transfer_prohibited;
        struct ast_channel *chan_bridged;
        struct ast_channel *chan_unbridged;
        int transfer_prohibited;

@@ -4412,8 +4419,10 @@ enum ast_transfer_result ast_bridge_transfer_attended(struct ast_channel *to_tra
        set_transfer_variables_all(to_transferee, channels, 1);
 
        if (do_bridge_transfer) {
        set_transfer_variables_all(to_transferee, channels, 1);
 
        if (do_bridge_transfer) {

-                res = attended_transfer_bridge(chan_bridged, chan_unbridged, the_bridge, NULL, &publication);
-                goto end;
+               ast_bridge_lock(the_bridge);
+               res = attended_transfer_bridge(chan_bridged, chan_unbridged, the_bridge, NULL, &publication);
+               ast_bridge_unlock(the_bridge);
+               goto end;

        }
 
        transferee = get_transferee(channels, chan_bridged);
        }
 
        transferee = get_transferee(channels, chan_bridged);

@@ -4430,7 +4439,9 @@ enum ast_transfer_result ast_bridge_transfer_attended(struct ast_channel *to_tra
 
        ast_bridge_remove(the_bridge, chan_bridged);
 
 
        ast_bridge_remove(the_bridge, chan_bridged);
 

+       ast_bridge_lock(the_bridge);

        publish_attended_transfer_app(&publication, app);
        publish_attended_transfer_app(&publication, app);

+       ast_bridge_unlock(the_bridge);

        res = AST_BRIDGE_TRANSFER_SUCCESS;
 
 end:
        res = AST_BRIDGE_TRANSFER_SUCCESS;
 
 end:

@@ -4438,7 +4449,20 @@ end:
         * All failure paths have deferred publishing a stasis message until this point
         */
        if (res != AST_BRIDGE_TRANSFER_SUCCESS) {
         * All failure paths have deferred publishing a stasis message until this point
         */
        if (res != AST_BRIDGE_TRANSFER_SUCCESS) {

+               if (to_transferee_bridge && to_target_bridge) {
+                       ast_bridge_lock_both(to_transferee_bridge, to_target_bridge);
+               } else if (the_bridge) {
+                       ast_bridge_lock(the_bridge);
+               }
+

                publish_attended_transfer_fail(&publication, res);
                publish_attended_transfer_fail(&publication, res);

+
+               if (to_transferee_bridge && to_target_bridge) {
+                       ast_bridge_unlock(to_transferee_bridge);
+                       ast_bridge_unlock(to_target_bridge);
+               } else if (the_bridge) {
+                       ast_bridge_unlock(the_bridge);
+               }

        }
        stasis_publish_data_cleanup(&publication);
        return res;
        }
        stasis_publish_data_cleanup(&publication);
        return res;

diff --git a/main/bridge_basic.c b/main/bridge_basic.c
index 4a68f49..253aa35 100644 (file)
--- a/main/bridge_basic.c
+++ b/main/bridge_basic.c
@@ -1534,8 +1534,11 @@ static void publish_transfer_success(struct attended_transfer_properties *props)
                .bridge = props->target_bridge,
        };
 
                .bridge = props->target_bridge,
        };
 

+       ast_bridge_lock_both(transferee.bridge, transfer_target.bridge);

        ast_bridge_publish_attended_transfer_bridge_merge(0, AST_BRIDGE_TRANSFER_SUCCESS,
                        &transferee, &transfer_target, props->transferee_bridge);
        ast_bridge_publish_attended_transfer_bridge_merge(0, AST_BRIDGE_TRANSFER_SUCCESS,
                        &transferee, &transfer_target, props->transferee_bridge);

+       ast_bridge_unlock(transferee.bridge);
+       ast_bridge_unlock(transfer_target.bridge);

 }
 
 /*!
 }
 
 /*!

@@ -1556,8 +1559,11 @@ static void publish_transfer_threeway(struct attended_transfer_properties *props
                .bridge = props->transferee_bridge,
        };
 
                .bridge = props->transferee_bridge,
        };
 

+       ast_bridge_lock_both(transferee.bridge, transfer_target.bridge);

        ast_bridge_publish_attended_transfer_threeway(0, AST_BRIDGE_TRANSFER_SUCCESS,
                        &transferee, &transfer_target, &threeway);
        ast_bridge_publish_attended_transfer_threeway(0, AST_BRIDGE_TRANSFER_SUCCESS,
                        &transferee, &transfer_target, &threeway);

+       ast_bridge_unlock(transferee.bridge);
+       ast_bridge_unlock(transfer_target.bridge);

 }
 
 /*!
 }
 
 /*!

@@ -1574,8 +1580,11 @@ static void publish_transfer_fail(struct attended_transfer_properties *props)
                .bridge = props->target_bridge,
        };
 
                .bridge = props->target_bridge,
        };
 

+       ast_bridge_lock_both(transferee.bridge, transfer_target.bridge);

        ast_bridge_publish_attended_transfer_fail(0, AST_BRIDGE_TRANSFER_FAIL,
                        &transferee, &transfer_target);
        ast_bridge_publish_attended_transfer_fail(0, AST_BRIDGE_TRANSFER_FAIL,
                        &transferee, &transfer_target);

+       ast_bridge_unlock(transferee.bridge);
+       ast_bridge_unlock(transfer_target.bridge);

 }
 
 /*!
 }
 
 /*!

diff --git a/res/ari/resource_bridges.c b/res/ari/resource_bridges.c
index bb39ad6..d78601f 100644 (file)
--- a/res/ari/resource_bridges.c
+++ b/res/ari/resource_bridges.c
@@ -746,7 +746,10 @@ void ast_ari_bridges_create(struct ast_variable *headers,
                return;
        }
 
                return;
        }
 

+       ast_bridge_lock(bridge);

        snapshot = ast_bridge_snapshot_create(bridge);
        snapshot = ast_bridge_snapshot_create(bridge);

+       ast_bridge_unlock(bridge);
+

        if (!snapshot) {
                ast_ari_response_error(
                        response, 500, "Internal Error",
        if (!snapshot) {
                ast_ari_response_error(
                        response, 500, "Internal Error",

@@ -792,7 +795,10 @@ void ast_ari_bridges_create_or_update_with_id(struct ast_variable *headers,
                return;
        }
 
                return;
        }
 

+       ast_bridge_lock(bridge);

        snapshot = ast_bridge_snapshot_create(bridge);
        snapshot = ast_bridge_snapshot_create(bridge);

+       ast_bridge_unlock(bridge);
+

        if (!snapshot) {
                ast_ari_response_error(
                        response, 500, "Internal Error",
        if (!snapshot) {
                ast_ari_response_error(
                        response, 500, "Internal Error",

diff --git a/tests/test_cel.c b/tests/test_cel.c
index de69008..791a73c 100644 (file)
--- a/tests/test_cel.c
+++ b/tests/test_cel.c
@@ -1213,8 +1213,10 @@ AST_TEST_DEFINE(test_cel_blind_transfer)
 
        pair.bridge = bridge;
        pair.channel = chan_alice;
 
        pair.bridge = bridge;
        pair.channel = chan_alice;

+       ast_bridge_lock(bridge);

        ast_bridge_publish_blind_transfer(1, AST_BRIDGE_TRANSFER_SUCCESS,
                &pair, "transfer_context", "transfer_extension");
        ast_bridge_publish_blind_transfer(1, AST_BRIDGE_TRANSFER_SUCCESS,
                &pair, "transfer_context", "transfer_extension");

+       ast_bridge_unlock(bridge);

        BLINDTRANSFER_EVENT(chan_alice, bridge, "transfer_extension", "transfer_context");
 
        BRIDGE_EXIT(chan_alice, bridge);
        BLINDTRANSFER_EVENT(chan_alice, bridge, "transfer_extension", "transfer_context");
 
        BRIDGE_EXIT(chan_alice, bridge);