[Asterisk-code-review] bridge native rtp.c: Fix reentrancy framehook crash. (asterisk[13])

[Jenkins2]

Jenkins2 has submitted this change and it was merged. ( https://gerrit.asterisk.org/7753 )

Change subject: bridge_native_rtp.c: Fix reentrancy framehook crash.
......................................................................

bridge_native_rtp.c: Fix reentrancy framehook crash.

If two channels enter different native rtp bridges at the same time it is
possible that the framehook interface data pointer can be corrupted
because the struct variable was declared static.

* Fixed the reentrancy corruption by changing the framehook interface
struct static variable to a stack local variable.

* Moved the hook.data assignment outside of the channel lock.  It did not
need the lock's protection.  It probably was giving a false sense of
security.

The testsuite
channels/pjsip/basic_calls/two_parties/nominal/alice_initiated/bob_hangs_up
test caught this with MALLOC_DEBUG and DO_CRASH enabled.

Change-Id: If9e35b97d19209b0f984941c1d8eb5f7c55eea91
---
M bridges/bridge_native_rtp.c
1 file changed, 3 insertions(+), 2 deletions(-)

Approvals:
  Corey Farrell: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, approved
  Jenkins2: Approved for Submit



diff --git a/bridges/bridge_native_rtp.c b/bridges/bridge_native_rtp.c
index 122c132..d490183 100644
--- a/bridges/bridge_native_rtp.c
+++ b/bridges/bridge_native_rtp.c
@@ -755,7 +755,7 @@
 static int native_rtp_bridge_framehook_attach(struct ast_bridge_channel *bridge_channel)
 {
 	struct native_rtp_bridge_channel_data *data = bridge_channel->tech_pvt;
-	static struct ast_framehook_interface hook = {
+	struct ast_framehook_interface hook = {
 		.version = AST_FRAMEHOOK_INTERFACE_VERSION,
 		.event_cb = native_rtp_framehook,
 		.destroy_cb = __ao2_cleanup,
@@ -773,9 +773,10 @@
 	ast_debug(2, "Bridge '%s'.  Attaching hook data %p to '%s'\n",
 		bridge_channel->bridge->uniqueid, data, ast_channel_name(bridge_channel->chan));
 
-	ast_channel_lock(bridge_channel->chan);
 	/* We're giving 1 ref to the framehook and keeping the one from the alloc for ourselves */
 	hook.data = ao2_bump(data->hook_data);
+
+	ast_channel_lock(bridge_channel->chan);
 	data->hook_data->id = ast_framehook_attach(bridge_channel->chan, &hook);
 	ast_channel_unlock(bridge_channel->chan);
 	if (data->hook_data->id < 0) {

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

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: merged
Gerrit-Change-Id: If9e35b97d19209b0f984941c1d8eb5f7c55eea91
Gerrit-Change-Number: 7753
Gerrit-PatchSet: 2
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171228/bc414e14/attachment.html>