[asterisk-commits] rmudgett: trunk r398939 - in /trunk: ./ main/core_unreal.c

[SVN commits to the Asterisk project]

Author: rmudgett
Date: Thu Sep 12 11:44:34 2013
New Revision: 398939

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=398939
Log:
core_local: Fix memory corruption race condition.

The masquerade super test is failing on v12 with high fence violations and
crashing.  The fence violations are showing that party id allocated memory
strings are somehow getting corrupted in the
bridge_reconfigured_connected_line_update() function.  The invalid string
values happen to be the freed memory fill pattern.

After much puzzling, I deduced that the
bridge_reconfigured_connected_line_update() is copying a string out of the
source channel's caller party id struct just as another thread is updating
it with a new value.  The copying thread is using the old string pointer
being freed by the updating thread.  A search of the code found the
unreal_colp_redirect_indicate() routine updating the caller party id's
without holding the channel lock.

A latent bug in v1.8 and v11 hatched in v12 because of the bridging and
connected line changes.  :)

(issue ASTERISK-22221)
Reported by: Matt Jordan

Review: https://reviewboard.asterisk.org/r/2839/
........

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

Modified:
    trunk/   (props changed)
    trunk/main/core_unreal.c

Propchange: trunk/
------------------------------------------------------------------------------
--- branch-12-merged (original)
+++ branch-12-merged Thu Sep 12 11:44:34 2013
@@ -1,1 +1,1 @@
-/branches/12:1-398558,398560-398577,398579-398927
+/branches/12:1-398558,398560-398577,398579-398927,398938

Modified: trunk/main/core_unreal.c
URL: http://svnview.digium.com/svn/asterisk/trunk/main/core_unreal.c?view=diff&rev=398939&r1=398938&r2=398939
==============================================================================
--- trunk/main/core_unreal.c (original)
+++ trunk/main/core_unreal.c Thu Sep 12 11:44:34 2013
@@ -441,10 +441,18 @@
  */
 static int unreal_colp_redirect_indicate(struct ast_unreal_pvt *p, struct ast_channel *ast, int condition)
 {
+	struct ast_channel *my_chan;
+	struct ast_channel *my_owner;
 	struct ast_channel *this_channel;
 	struct ast_channel *the_other_channel;
 	int isoutbound;
 	int res = 0;
+	unsigned char frame_data[1024];
+	struct ast_frame f = {
+		.frametype = AST_FRAME_CONTROL,
+		.subclass.integer = condition,
+		.data.ptr = frame_data,
+	};
 
 	/*
 	 * A connected line update frame may only contain a partial
@@ -458,7 +466,8 @@
 	 * redirecting information, which is why it is handled here as
 	 * well.
 	 */
-	ao2_lock(p);
+	ast_channel_unlock(ast);
+	ast_unreal_lock_all(p, &my_chan, &my_owner);
 	isoutbound = AST_UNREAL_IS_OUTBOUND(ast, p);
 	if (isoutbound) {
 		this_channel = p->chan;
@@ -468,13 +477,6 @@
 		the_other_channel = p->chan;
 	}
 	if (the_other_channel) {
-		unsigned char frame_data[1024];
-		struct ast_frame f = {
-			.frametype = AST_FRAME_CONTROL,
-			.subclass.integer = condition,
-			.data.ptr = frame_data,
-		};
-
 		if (condition == AST_CONTROL_CONNECTED_LINE) {
 			ast_connected_line_copy_to_caller(ast_channel_caller(the_other_channel),
 				ast_channel_connected(this_channel));
@@ -484,9 +486,20 @@
 			f.datalen = ast_redirecting_build_data(frame_data, sizeof(frame_data),
 				ast_channel_redirecting(this_channel), NULL);
 		}
-		res = unreal_queue_frame(p, isoutbound, &f, ast, 1);
-	}
-	ao2_unlock(p);
+	}
+	if (my_chan) {
+		ast_channel_unlock(my_chan);
+		ast_channel_unref(my_chan);
+	}
+	if (my_owner) {
+		ast_channel_unlock(my_owner);
+		ast_channel_unref(my_owner);
+	}
+	if (the_other_channel) {
+		res = unreal_queue_frame(p, isoutbound, &f, ast, 0);
+	}
+	ao2_unlock(p);
+	ast_channel_lock(ast);
 
 	return res;
 }