[asterisk-commits] mjordan: trunk r392564 - /trunk/res/res_fax.c

SVN commits to the Asterisk project asterisk-commits at lists.digium.com
Sat Jun 22 08:58:09 CDT 2013


Author: mjordan
Date: Sat Jun 22 08:58:07 2013
New Revision: 392564

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=392564
Log:
Fix a deadlock and possible crash in res_fax

This patch fixes two bugs.
(1) It unlocks the channel in the framehook handlers before attempting to grab
    the peer from the bridge. The locking order for the bridging framework is
    bridge first, then channel - having the channel locked while attempting to
    obtain the bridge lock causes a locking inversion and a deadlock. This
    patch bumps the channel ref count prior to releasing the lock in the
    framehook to avoid lifetime issues.

    Note that this does expose a subtle problem in framehooks; that is,
    something could modify the framehook list while we are executing, causing
    issues in the framehook list traversal that the callback executes in.
    Fixing this is a much larger problem that is beyond the scope of this
    patch - (a) we already unlock the channel in this particular framehook
    and we haven't run into a problem yet (as modifying the framehook list
    when a channel is about to perform a fax gateway would be a very odd
    operation) and (b) migrating to an ao2 container of framehooks would be
    more invasive at this point. See the referenced ASTERISK issue for more
    information.
(2) Directly packing channel variables into a JSON object turned out to be
    unsafe. A condition existed where the strings in the JSON blob were no
    longer safe to be accessed if the channel object itself was disposed of.

(issue ASTERISK-21951)

Modified:
    trunk/res/res_fax.c

Modified: trunk/res/res_fax.c
URL: http://svnview.digium.com/svn/asterisk/trunk/res/res_fax.c?view=diff&rev=392564&r1=392563&r2=392564
==============================================================================
--- trunk/res/res_fax.c (original)
+++ trunk/res/res_fax.c Sat Jun 22 08:58:07 2013
@@ -58,7 +58,7 @@
  * \addtogroup configuration_file Configuration Files
  */
 
-/*! 
+/*!
  * \page res_fax.conf res_fax.conf
  * \verbinclude res_fax.conf.sample
  */
@@ -1775,15 +1775,26 @@
 	ast_json_array_append(json_array, json_filename);
 
 	{
+		const char *remote_station_id;
+		const char *local_station_id;
+		const char *fax_pages;
+		const char *fax_resolution;
+		const char *fax_bitrate;
 		SCOPED_CHANNELLOCK(lock, chan);
 
-		json_object = ast_json_pack("s: s, s: s, s: s, s: s, s: s, s: s, s: o",
-				"type", "receive"
-				"remote_station_id", S_OR(pbx_builtin_getvar_helper(chan, "REMOTESTATIONID"), ""),
-				"local_station_id", S_OR(pbx_builtin_getvar_helper(chan, "LOCALSTATIONID"), ""),
-				"fax_pages", S_OR(pbx_builtin_getvar_helper(chan, "FAXPAGES"), ""),
-				"fax_resolution", S_OR(pbx_builtin_getvar_helper(chan, "FAXRESOLUTION"), ""),
-				"fax_bitrate", S_OR(pbx_builtin_getvar_helper(chan, "FAXBITRATE"), ""),
+		remote_station_id = S_OR(pbx_builtin_getvar_helper(chan, "REMOTESTATIONID"), "");
+		local_station_id = S_OR(pbx_builtin_getvar_helper(chan, "LOCALSTATIONID"), "");
+		fax_pages = S_OR(pbx_builtin_getvar_helper(chan, "FAXPAGES"), "");
+		fax_resolution = S_OR(pbx_builtin_getvar_helper(chan, "FAXRESOLUTION"), "");
+		fax_bitrate = S_OR(pbx_builtin_getvar_helper(chan, "FAXBITRATE"), "");
+
+		json_object = ast_json_pack("{s: s, s: s, s: s, s: s, s: s, s: s, s: O}",
+				"type", "receive",
+				"remote_station_id", remote_station_id,
+				"local_station_id", local_station_id,
+				"fax_pages", fax_pages,
+				"fax_resolution", fax_resolution,
+				"fax_bitrate", fax_bitrate,
 				"filenames", json_array);
 		if (!json_object) {
 			return -1;
@@ -1793,7 +1804,6 @@
 		if (!message) {
 			return -1;
 		}
-
 		stasis_publish(ast_channel_topic(chan), message);
 	}
 	return 0;
@@ -2256,14 +2266,26 @@
 	}
 
 	{
+		const char *remote_station_id;
+		const char *local_station_id;
+		const char *fax_pages;
+		const char *fax_resolution;
+		const char *fax_bitrate;
 		SCOPED_CHANNELLOCK(lock, chan);
+
+		remote_station_id = S_OR(pbx_builtin_getvar_helper(chan, "REMOTESTATIONID"), "");
+		local_station_id = S_OR(pbx_builtin_getvar_helper(chan, "LOCALSTATIONID"), "");
+		fax_pages = S_OR(pbx_builtin_getvar_helper(chan, "FAXPAGES"), "");
+		fax_resolution = S_OR(pbx_builtin_getvar_helper(chan, "FAXRESOLUTION"), "");
+		fax_bitrate = S_OR(pbx_builtin_getvar_helper(chan, "FAXBITRATE"), "");
+
 		json_obj = ast_json_pack("{s: s, s: s, s: s, s: s, s: s, s: s, s: o}",
 				"type", "send"
-				"remote_station_id", S_OR(pbx_builtin_getvar_helper(chan, "REMOTESTATIONID"), ""),
-				"local_station_id", S_OR(pbx_builtin_getvar_helper(chan, "LOCALSTATIONID"), ""),
-				"fax_pages", S_OR(pbx_builtin_getvar_helper(chan, "FAXPAGES"), ""),
-				"fax_resolution", S_OR(pbx_builtin_getvar_helper(chan, "FAXRESOLUTION"), ""),
-				"fax_bitrate", S_OR(pbx_builtin_getvar_helper(chan, "FAXBITRATE"), ""),
+				"remote_station_id", remote_station_id,
+				"local_station_id", local_station_id,
+				"fax_pages", fax_pages,
+				"fax_resolution", fax_resolution,
+				"fax_bitrate", fax_bitrate,
 				"filenames", json_filenames);
 		if (!json_obj) {
 			return -1;
@@ -2980,6 +3002,10 @@
 	struct ast_channel *active;
 	RAII_VAR(struct ast_fax_session_details *, details, NULL, ao2_cleanup);
 	RAII_VAR(struct ast_channel *, peer, NULL, ao2_cleanup);
+	RAII_VAR(struct ast_channel *, chan_ref, chan, ao2_cleanup);
+
+	/* Ref bump channel for when we have to unlock it */
+	ao2_ref(chan_ref, 1);
 
 	if (gateway->s) {
 		details = gateway->s->details;
@@ -3000,7 +3026,10 @@
 			ast_set_read_format(chan, &gateway->chan_read_format);
 			ast_set_read_format(chan, &gateway->chan_write_format);
 
-			if ((peer = ast_channel_bridge_peer(chan))) {
+			ast_channel_unlock(chan);
+			peer = ast_channel_bridge_peer(chan);
+			ast_channel_lock(chan);
+			if (peer) {
 				ast_set_read_format(peer, &gateway->peer_read_format);
 				ast_set_read_format(peer, &gateway->peer_write_format);
 				ast_channel_make_compatible(chan, peer);
@@ -3019,7 +3048,10 @@
 	}
 
 	/* If we aren't bridged or we don't have a peer, don't do anything */
-	if (!(peer = ast_channel_bridge_peer(chan))) {
+	ast_channel_unlock(chan);
+	peer = ast_channel_bridge_peer(chan);
+	ast_channel_lock(chan);
+	if (!peer) {
 		return f;
 	}
 
@@ -3290,7 +3322,11 @@
 	struct ast_fax_session_details *details;
 	struct ast_control_t38_parameters *control_params;
 	RAII_VAR(struct ast_channel *, peer, NULL, ao2_cleanup);
+	RAII_VAR(struct ast_channel *, chan_ref, chan, ao2_cleanup);
 	int result = 0;
+
+	/* Ref bump the channel for when we have to unlock it */
+	ao2_ref(chan, 1);
 
 	details = faxdetect->details;
 
@@ -3314,7 +3350,10 @@
 	case AST_FRAMEHOOK_EVENT_DETACHED:
 		/* restore audio formats when we are detached */
 		ast_set_read_format(chan, &faxdetect->orig_format);
-		if ((peer = ast_channel_bridge_peer(chan))) {
+		ast_channel_unlock(chan);
+		peer = ast_channel_bridge_peer(chan);
+		ast_channel_lock(chan);
+		if (peer) {
 			ast_channel_make_compatible(chan, peer);
 		}
 		return NULL;
@@ -4138,8 +4177,8 @@
  * Module loading including tests for configuration or dependencies.
  * This function can return AST_MODULE_LOAD_FAILURE, AST_MODULE_LOAD_DECLINE,
  * or AST_MODULE_LOAD_SUCCESS. If a dependency or environment variable fails
- * tests return AST_MODULE_LOAD_FAILURE. If the module can not load the 
- * configuration file or other non-critical problem return 
+ * tests return AST_MODULE_LOAD_FAILURE. If the module can not load the
+ * configuration file or other non-critical problem return
  * AST_MODULE_LOAD_DECLINE. On success return AST_MODULE_LOAD_SUCCESS.
  */
 static int load_module(void)




More information about the asterisk-commits mailing list