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

[Joshua Colp]

Joshua Colp has uploaded a new change for review.

  https://gerrit.asterisk.org/855

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. This allows the frame hook to
determine whether it has been detached as a field in the native RTP
bridge data is set when it has been. If the frame hook has been
detached then native RTP bridging is not started.

ASTERISK-25240 #close

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


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/55/855/1

diff --git a/bridges/bridge_native_rtp.c b/bridges/bridge_native_rtp.c
index d5652bc..e47d86c 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,21 @@
 		/* 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);
@@ -396,6 +407,12 @@
 	return 1;
 }
 
+/*! \brief Destroy function for frame hook */
+static void native_rtp_framehook_destroy(void *data)
+{
+	ao2_ref(data, -1);
+}
+
 /*! \brief Helper function which adds frame hook to bridge channel */
 static int native_rtp_bridge_framehook_attach(struct ast_bridge_channel *bridge_channel)
 {
@@ -403,6 +420,7 @@
 	static struct ast_framehook_interface hook = {
 		.version = AST_FRAMEHOOK_INTERFACE_VERSION,
 		.event_cb = native_rtp_framehook,
+		.destroy_cb = native_rtp_framehook_destroy,
 		.consume_cb = native_rtp_framehook_consume,
 		.disable_inheritance = 1,
 	};
@@ -412,10 +430,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 +455,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/855
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I13a73186a05f4e5a764f81e5cd0ccec1ed1891d2
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Joshua Colp <jcolp at digium.com>