[Asterisk-code-review] stasis transfer: fix a race condition on stasis bridge push (asterisk[certified/13.1])

[Richard Mudgett]

Richard Mudgett has uploaded a new change for review.

  https://gerrit.asterisk.org/2664

Change subject: stasis transfer: fix a race condition on stasis bridge push
......................................................................

stasis transfer: fix a race condition on stasis bridge push

After a bridge transfer completes where a local replacement
channel is used, a stasis transfer message with the details
of the transfer is sent.  This is processed by stasis which
then sets the stasis app name and replaced channel snapshot
on the replacement channel.

However, since a separate thread was already started to run
stasis on the new replacement channel, a race was on to see
if the message processing would be completed before the app
name was needed, otherwise the channel would be hung up.

This change moves the calls used to set the stasis app name
and the replace snapshot to the bridge_stasis_push function
callback from the bridge transfer logic, allowing the steps
to be completed earlier and more deterministically, and the
race elimianted.

NOTE: the swap channel parameter to bridge_stasis_push (and
thus all bridge push callbacks) must always be present when
performing a swap with another channel.

ASTERISK-24649 #close
Reported by: John Bigelow
Review: https://reviewboard.asterisk.org/r/4341/

This patch is a remedial cherry-pick from v13.

Change-Id: I35c98989786f74cdd7940677002a1a88d34bd2dd
---
M res/stasis/app.c
M res/stasis/stasis_bridge.c
2 files changed, 29 insertions(+), 34 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/64/2664/1

diff --git a/res/stasis/app.c b/res/stasis/app.c
index 1cc4fb5..330e711 100644
--- a/res/stasis/app.c
+++ b/res/stasis/app.c
@@ -724,33 +724,12 @@
 	return subscribed;
 }
 
-static void set_replacement_channel(struct ast_channel_snapshot *to_be_replaced,
-		struct ast_channel_snapshot *replacing)
-{
-	struct stasis_app_control *control = stasis_app_control_find_by_channel_id(
-		to_be_replaced->uniqueid);
-	struct ast_channel *chan = ast_channel_get_by_name(replacing->uniqueid);
-
-	if (control && chan) {
-		ast_channel_lock(chan);
-		app_set_replace_channel_app(chan, app_name(control_app(control)));
-		app_set_replace_channel_snapshot(chan, to_be_replaced);
-		ast_channel_unlock(chan);
-	}
-	ast_channel_cleanup(chan);
-	ao2_cleanup(control);
-}
-
 static void bridge_blind_transfer_handler(void *data, struct stasis_subscription *sub,
 	struct stasis_message *message)
 {
 	struct stasis_app *app = data;
 	struct ast_blind_transfer_message *transfer_msg = stasis_message_data(message);
 	struct ast_bridge_snapshot *bridge = transfer_msg->bridge;
-
-	if (transfer_msg->replace_channel) {
-		set_replacement_channel(transfer_msg->transferer, transfer_msg->replace_channel);
-	}
 
 	if (bridge_app_subscribed(app, transfer_msg->transferer->uniqueid) ||
 		(bridge && bridge_app_subscribed_involved(app, bridge))) {
@@ -801,18 +780,6 @@
 
 	if (subscribed) {
 		stasis_publish(app->topic, message);
-	}
-
-	if (transfer_msg->replace_channel) {
-		set_replacement_channel(transfer_msg->to_transferee.channel_snapshot,
-				transfer_msg->replace_channel);
-	}
-
-	if (transfer_msg->dest_type == AST_ATTENDED_TRANSFER_DEST_LINK) {
-		set_replacement_channel(transfer_msg->to_transferee.channel_snapshot,
-				transfer_msg->dest.links[0]);
-		set_replacement_channel(transfer_msg->to_transfer_target.channel_snapshot,
-				transfer_msg->dest.links[1]);
 	}
 }
 
diff --git a/res/stasis/stasis_bridge.c b/res/stasis/stasis_bridge.c
index 646b306..6572384 100644
--- a/res/stasis/stasis_bridge.c
+++ b/res/stasis/stasis_bridge.c
@@ -115,6 +115,27 @@
 {
 	struct stasis_app_control *control = stasis_app_control_find_by_channel(bridge_channel->chan);
 
+	if (swap) {
+		struct ast_channel_snapshot *to_be_replaced = ast_channel_snapshot_get_latest(ast_channel_uniqueid(swap->chan));
+		struct stasis_app_control *swap_control = stasis_app_control_find_by_channel(swap->chan);
+
+		/* set the replace channel snapshot */
+		ast_channel_lock(bridge_channel->chan);
+		app_set_replace_channel_snapshot(bridge_channel->chan, to_be_replaced);
+
+		/* copy the app name from the swap channel */
+		if (swap_control) {
+			ast_debug(3, "Copying stasis app name %s from %s to %s\n",
+				app_name(control_app(swap_control)),
+				ast_channel_name(swap->chan),
+				ast_channel_name(bridge_channel->chan));
+			app_set_replace_channel_app(bridge_channel->chan, app_name(control_app(swap_control)));
+		}
+		ast_channel_unlock(bridge_channel->chan);
+		ao2_cleanup(to_be_replaced);
+		ao2_cleanup(swap_control);
+	}
+
 	if (!control && !stasis_app_channel_is_internal(bridge_channel->chan)) {
 		/* channel not in Stasis(), get it there */
 		/* Attach after-bridge callback and pass ownership of swap_app to it */
@@ -125,8 +146,15 @@
 		}
 
 		bridge_stasis_queue_join_action(self, bridge_channel);
+		if (swap) {
+			/* nudge the swap channel out of the bridge */
+			ast_bridge_channel_leave_bridge(swap, BRIDGE_CHANNEL_STATE_END_NO_DISSOLVE, 0);
+		}
 
-		/* Return -1 so the push fails and the after-bridge callback gets called */
+		/* Return -1 so the push fails and the after-bridge callback gets called
+		 * This keeps the bridging framework from putting the channel into the bridge
+		 * until the Stasis thread gets started, and then the channel is put into the bridge.
+		 */
 		return -1;
 	}
 

-- 
To view, visit https://gerrit.asterisk.org/2664
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I35c98989786f74cdd7940677002a1a88d34bd2dd
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: certified/13.1
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Scott Griepentrog <sgriepentrog at digium.com>