[Asterisk-code-review] bridge native rtp.c: Don't start native RTP bridging after a... (asterisk[master])

[Matt Jordan]

Matt Jordan has submitted this change and it was merged.

Change subject: bridge_native_rtp.c: Don't start native RTP bridging after attended transfer.
......................................................................


bridge_native_rtp.c: Don't start native RTP bridging after attended transfer.

The bridge_native_rtp module adds a frame hook to channels which are in
a native RTP bridge. This frame hook is used to intercept when a hold
or unhold frame traverses the bridge so native RTP can be stopped or
started as appropriate. This is expected but exposes a specific bug
when attended transfers are involved.

Upon completion of an attended transfer an unhold frame is queued up
to take one of the channels involved off hold. After this is done
the channel is moved between bridges.

When the frame hook is involved in this case for the unhold it
releases the channel lock and acquires the bridge lock. This
allows the bridge core to step in and move the channel
(potentially changing the bridging techology) from another thread.
Once completed the bridge lock is released by the bridge core.
The frame hook is then able to acquire the bridge lock and
wrongfully starts native RTP again, despite the channel no longer
being in the bridge or needing to start native RTP. In fact at
this point the frame hook is no longer attached to the channel.

This change makes it so the native RTP bridge data is available to
the frame hook when it is invoked. Whether the frame hook has
been detached or not is stored on the native RTP bridge data and
is checked by the frame hook before starting or stopping native
RTP bridging. If the frame hook has been detached it does nothing.

ASTERISK-25240 #close

Change-Id: I13a73186a05f4e5a764f81e5cd0ccec1ed1891d2
---
M bridges/bridge_native_rtp.c
1 file changed, 22 insertions(+), 6 deletions(-)

Approvals:
  Richard Mudgett: Looks good to me, but someone else must approve
  Anonymous Coward #1000019: Verified
  Matt Jordan: Looks good to me, approved



diff --git a/bridges/bridge_native_rtp.c b/bridges/bridge_native_rtp.c
index dc0c741..606a43a 100644
--- a/bridges/bridge_native_rtp.c
+++ b/bridges/bridge_native_rtp.c
@@ -50,6 +50,8 @@
 struct native_rtp_bridge_data {
 	/*! \brief Framehook used to intercept certain control frames */
 	int id;
+	/*! \brief Set when this framehook has been detached */
+	unsigned int detached;
 };
 
 /*! \brief Internal helper function which gets all RTP information (glue and instances) relating to the given channels */
@@ -261,6 +263,7 @@
 static struct ast_frame *native_rtp_framehook(struct ast_channel *chan, struct ast_frame *f, enum ast_framehook_event event, void *data)
 {
 	RAII_VAR(struct ast_bridge *, bridge, NULL, ao2_cleanup);
+	struct native_rtp_bridge_data *native_data = data;
 
 	if (!f || (event != AST_FRAMEHOOK_EVENT_WRITE)) {
 		return f;
@@ -272,13 +275,22 @@
 		/* native_rtp_bridge_start/stop are not being called from bridging
 		   core so we need to lock the bridge prior to calling these functions
 		   Unfortunately that means unlocking the channel, but as it
-		   should not be modified this should be okay...hopefully */
+		   should not be modified this should be okay... hopefully...
+		   unless this channel is being moved around right now and is in
+		   the process of having this framehook removed (which is fine). To
+		   ensure we then don't stop or start when we shouldn't we consult
+		   the data provided. If this framehook has been detached then the
+		   detached variable will be set. This is safe to check as it is only
+		   manipulated with the bridge lock held. */
 		ast_channel_unlock(chan);
 		ast_bridge_lock(bridge);
-		if (f->subclass.integer == AST_CONTROL_HOLD) {
-			native_rtp_bridge_stop(bridge, chan);
-		} else if ((f->subclass.integer == AST_CONTROL_UNHOLD) || (f->subclass.integer == AST_CONTROL_UPDATE_RTP_PEER)) {
-			native_rtp_bridge_start(bridge, chan);
+		if (!native_data->detached) {
+			if (f->subclass.integer == AST_CONTROL_HOLD) {
+				native_rtp_bridge_stop(bridge, chan);
+			} else if ((f->subclass.integer == AST_CONTROL_UNHOLD) ||
+				(f->subclass.integer == AST_CONTROL_UPDATE_RTP_PEER)) {
+				native_rtp_bridge_start(bridge, chan);
+			}
 		}
 		ast_bridge_unlock(bridge);
 		ast_channel_lock(chan);
@@ -403,6 +415,7 @@
 	static struct ast_framehook_interface hook = {
 		.version = AST_FRAMEHOOK_INTERFACE_VERSION,
 		.event_cb = native_rtp_framehook,
+		.destroy_cb = __ao2_cleanup,
 		.consume_cb = native_rtp_framehook_consume,
 		.disable_inheritance = 1,
 	};
@@ -412,10 +425,12 @@
 	}
 
 	ast_channel_lock(bridge_channel->chan);
+	hook.data = ao2_bump(data);
 	data->id = ast_framehook_attach(bridge_channel->chan, &hook);
 	ast_channel_unlock(bridge_channel->chan);
 	if (data->id < 0) {
-		ao2_cleanup(data);
+		/* We need to drop both the reference we hold, and the one the framehook would hold */
+		ao2_ref(data, -2);
 		return -1;
 	}
 
@@ -435,6 +450,7 @@
 
 	ast_channel_lock(bridge_channel->chan);
 	ast_framehook_detach(bridge_channel->chan, data->id);
+	data->detached = 1;
 	ast_channel_unlock(bridge_channel->chan);
 	bridge_channel->tech_pvt = NULL;
 }

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I13a73186a05f4e5a764f81e5cd0ccec1ed1891d2
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>