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)


git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@392564 65c4cc65-6c06-0410-ace0-fbb531ad65f3

1 files changed, 58 insertions, 19 deletions

diff --git a/res/res_fax.c b/res/res_fax.c
index 4c53a0d7e..f3a794ba9 100644
--- a/res/res_fax.c
+++ b/res/res_fax.c
@@ -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 @@ static int report_receive_fax_status(struct ast_channel *chan, const char *filen
 	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 @@ static int report_receive_fax_status(struct ast_channel *chan, const char *filen
 		if (!message) {
 			return -1;
 		}
-
 		stasis_publish(ast_channel_topic(chan), message);
 	}
 	return 0;
@@ -2256,14 +2266,26 @@ static int report_send_fax_status(struct ast_channel *chan, struct ast_fax_sessi
 	}
 
 	{
+		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 @@ static struct ast_frame *fax_gateway_framehook(struct ast_channel *chan, struct
 	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 @@ static struct ast_frame *fax_gateway_framehook(struct ast_channel *chan, struct
 			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 @@ static struct ast_frame *fax_gateway_framehook(struct ast_channel *chan, struct
 	}
 
 	/* 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,8 +3322,12 @@ static struct ast_frame *fax_detect_framehook(struct ast_channel *chan, struct a
 	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;
 
 	switch (event) {
@@ -3314,7 +3350,10 @@ static struct ast_frame *fax_detect_framehook(struct ast_channel *chan, struct a
 	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 @@ static int unload_module(void)
  * 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)