[Asterisk-code-review] res fax: Fix deadlock setting FAXMODE channel variable. (asterisk[master])

Richard Mudgett asteriskteam at digium.com
Thu Aug 25 18:25:58 CDT 2016


Richard Mudgett has uploaded a new change for review.

  https://gerrit.asterisk.org/3722

Change subject: res_fax: Fix deadlock setting FAXMODE channel variable.
......................................................................

res_fax: Fix deadlock setting FAXMODE channel variable.

ASTERISK-25980 added the FAXMODE channel variable to res_fax.c.
Unfortunately, it also introduced a deadlock potential because
set_channel_variables() which sets FAXMODE can be called during a
masquerade.  The ast_channel_get_t38_state() which gets the value used to
set FAXMODE cannot be called with the channel locked.  As a result, local
channels can deadlock because of how they must acquire the locks necessary
to operate.

The intent of FAXMODE is for dialplan to know how a fax was transferred
after the fax completes.  However, the previous patch sets FAXMODE to the
channel's current T.38 state AFTER the fax has completed and where T.38
may have already disconnected.

* Set FAXMODE based upon T.38 negotiations exchanged either with the fax
applications or the fax framehooks.

ASTERISK-26203
Reported by: Etienne Lessard

ASTERISK-24822
Reported by: David Brillert

ASTERISK-22732
Reported by: Richard Mudgett

Change-Id: Id525747254b64c1efe8b1b5973d52ff9719c2ae1
---
M include/asterisk/res_fax.h
M res/res_fax.c
2 files changed, 43 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/22/3722/1

diff --git a/include/asterisk/res_fax.h b/include/asterisk/res_fax.h
index 5119bfa..e88d800 100644
--- a/include/asterisk/res_fax.h
+++ b/include/asterisk/res_fax.h
@@ -20,16 +20,16 @@
 #ifndef _ASTERISK_RES_FAX_H
 #define _ASTERISK_RES_FAX_H
 
-#include <asterisk.h>
-#include <asterisk/lock.h>
-#include <asterisk/linkedlists.h>
-#include <asterisk/module.h>
-#include <asterisk/utils.h>
-#include <asterisk/options.h>
-#include <asterisk/frame.h>
-#include <asterisk/cli.h>
-#include <asterisk/stringfields.h>
-#include <asterisk/manager.h>
+#include "asterisk.h"
+#include "asterisk/lock.h"
+#include "asterisk/linkedlists.h"
+#include "asterisk/module.h"
+#include "asterisk/utils.h"
+#include "asterisk/options.h"
+#include "asterisk/frame.h"
+#include "asterisk/cli.h"
+#include "asterisk/stringfields.h"
+#include "asterisk/manager.h"
 
 /*! \brief capabilities for res_fax to locate a fax technology module */
 enum ast_fax_capabilities {
@@ -187,6 +187,8 @@
 	int faxdetect_timeout;
 	/*! flags used for fax detection */
 	int faxdetect_flags;
+	/*! Non-zero if T.38 is negotiated */
+	int is_t38_negotiated;
 };
 
 struct ast_fax_tech;
diff --git a/res/res_fax.c b/res/res_fax.c
index 5bbc896..a369d68 100644
--- a/res/res_fax.c
+++ b/res/res_fax.c
@@ -641,6 +641,7 @@
 		struct ast_fax_session_details *new_details = find_or_create_details(new_chan);
 
 		ast_framehook_detach(old_chan, old_details->gateway_id);
+		new_details->is_t38_negotiated = old_details->is_t38_negotiated;
 		fax_gateway_attach(new_chan, new_details);
 		ao2_cleanup(new_details);
 	}
@@ -1439,6 +1440,7 @@
 static void set_channel_variables(struct ast_channel *chan, struct ast_fax_session_details *details)
 {
 	char buf[10];
+
 	pbx_builtin_setvar_helper(chan, "FAXSTATUS", S_OR(details->result, NULL));
 	pbx_builtin_setvar_helper(chan, "FAXERROR", S_OR(details->error, NULL));
 	pbx_builtin_setvar_helper(chan, "FAXSTATUSSTRING", S_OR(details->resultstr, NULL));
@@ -1447,7 +1449,7 @@
 	pbx_builtin_setvar_helper(chan, "FAXBITRATE", S_OR(details->transfer_rate, NULL));
 	pbx_builtin_setvar_helper(chan, "FAXRESOLUTION", S_OR(details->resolution, NULL));
 
-	if (ast_channel_get_t38_state(chan) == T38_STATE_NEGOTIATED) {
+	if (details->is_t38_negotiated) {
 		pbx_builtin_setvar_helper(chan, "FAXMODE", "T38");
 	} else {
 		pbx_builtin_setvar_helper(chan, "FAXMODE", "audio");
@@ -1656,6 +1658,7 @@
 	ast_string_field_set(details, result, "");
 	ast_string_field_set(details, resultstr, "");
 	ast_string_field_set(details, error, "");
+	details->is_t38_negotiated = t38negotiated;
 	set_channel_variables(chan, details);
 
 	if (fax->tech->start_session(fax) < 0) {
@@ -1706,12 +1709,18 @@
 					 * do T.38 as well
 					 */
 					t38_parameters_fax_to_ast(&t38_parameters, &details->our_t38_parameters);
-					t38_parameters.request_response = (details->caps & AST_FAX_TECH_T38) ? AST_T38_NEGOTIATED : AST_T38_REFUSED;
+					if (details->caps & AST_FAX_TECH_T38) {
+						details->is_t38_negotiated = 1;
+						t38_parameters.request_response = AST_T38_NEGOTIATED;
+					} else {
+						t38_parameters.request_response = AST_T38_REFUSED;
+					}
 					ast_indicate_data(chan, AST_CONTROL_T38_PARAMETERS, &t38_parameters, sizeof(t38_parameters));
 					break;
 				case AST_T38_NEGOTIATED:
 					t38_parameters_ast_to_fax(&details->their_t38_parameters, parameters);
 					t38negotiated = 1;
+					details->is_t38_negotiated = 1;
 					break;
 				default:
 					break;
@@ -2902,6 +2911,7 @@
 		ast_string_field_set(details, result, "FAILED");
 		ast_string_field_set(details, resultstr, "error starting gateway session");
 		ast_string_field_set(details, error, "INIT_ERROR");
+		details->is_t38_negotiated = 0;
 		set_channel_variables(chan, details);
 		report_fax_status(chan, details, "No Available Resource");
 		ast_log(LOG_ERROR, "Can't create a FAX session, gateway attempt failed.\n");
@@ -2922,6 +2932,7 @@
 		ast_string_field_set(details, result, "FAILED");
 		ast_string_field_set(details, resultstr, "error starting gateway session");
 		ast_string_field_set(details, error, "INIT_ERROR");
+		details->is_t38_negotiated = 0;
 		set_channel_variables(chan, details);
 		return -1;
 	}
@@ -2967,6 +2978,7 @@
 
 	gateway->t38_state = T38_STATE_NEGOTIATING;
 	gateway->timeout_start = ast_tvnow();
+	details->is_t38_negotiated = 0;
 	details->gateway_timeout = FAX_GATEWAY_TIMEOUT;
 
 	ast_debug(1, "requesting T.38 for gateway session for %s\n", ast_channel_name(chan));
@@ -3064,6 +3076,7 @@
 			t38_parameters_ast_to_fax(&details->their_t38_parameters, control_params);
 			gateway->t38_state = T38_STATE_UNKNOWN;
 			gateway->timeout_start = ast_tvnow();
+			details->is_t38_negotiated = 0;
 			details->gateway_timeout = FAX_GATEWAY_TIMEOUT;
 			ao2_ref(details, -1);
 			return f;
@@ -3079,12 +3092,14 @@
 			if (fax_gateway_start(gateway, details, chan)) {
 				ast_log(LOG_ERROR, "error starting T.38 gateway for T.38 channel %s and G.711 channel %s\n", ast_channel_name(active), ast_channel_name(other));
 				gateway->t38_state = T38_STATE_REJECTED;
+				details->is_t38_negotiated = 0;
 				control_params->request_response = AST_T38_REFUSED;
 
 				ast_framehook_detach(chan, details->gateway_id);
 				details->gateway_id = -1;
 			} else {
 				gateway->t38_state = T38_STATE_NEGOTIATED;
+				details->is_t38_negotiated = chan == active;
 				control_params->request_response = AST_T38_NEGOTIATED;
 				report_fax_status(chan, details, "T.38 Negotiated");
 			}
@@ -3102,6 +3117,7 @@
 			t38_parameters_ast_to_fax(&details->their_t38_parameters, control_params);
 			gateway->t38_state = T38_STATE_UNKNOWN;
 			gateway->timeout_start = ast_tvnow();
+			details->is_t38_negotiated = 0;
 			details->gateway_timeout = FAX_GATEWAY_TIMEOUT;
 
 			ast_debug(1, "%s is attempting to negotiate T.38 after we already sent a negotiation request based on v21 preamble detection\n", ast_channel_name(active));
@@ -3124,6 +3140,7 @@
 			ast_string_field_set(details, result, "SUCCESS");
 			ast_string_field_set(details, resultstr, "no gateway necessary");
 			ast_string_field_set(details, error, "NATIVE_T38");
+			details->is_t38_negotiated = 1;
 			set_channel_variables(chan, details);
 
 			ast_debug(1, "%s is attempting to negotiate T.38 after we already negotiated T.38 with %s, disabling the gateway\n", ast_channel_name(active), ast_channel_name(other));
@@ -3138,6 +3155,7 @@
 		&& control_params->request_response == AST_T38_REFUSED) {
 
 		ast_debug(1, "unable to negotiate T.38 on %s for fax gateway\n", ast_channel_name(active));
+		details->is_t38_negotiated = 0;
 
 		/* our request to negotiate T.38 was refused, if the other
 		 * channel supports T.38, they might still reinvite and save
@@ -3166,11 +3184,13 @@
 		if (fax_gateway_start(gateway, details, chan)) {
 			ast_log(LOG_ERROR, "error starting T.38 gateway for T.38 channel %s and G.711 channel %s\n", ast_channel_name(active), ast_channel_name(other));
 			gateway->t38_state = T38_STATE_NEGOTIATING;
+			details->is_t38_negotiated = 0;
 			control_params->request_response = AST_T38_REQUEST_TERMINATE;
 
 			fax_gateway_indicate_t38(chan, active, control_params);
 		} else {
 			gateway->t38_state = T38_STATE_NEGOTIATED;
+			details->is_t38_negotiated = chan == active;
 			report_fax_status(chan, details, "T.38 Negotiated");
 		}
 
@@ -3186,14 +3206,16 @@
 		t38_parameters_fax_to_ast(control_params, &details->our_t38_parameters);
 
 		if (fax_gateway_start(gateway, details, chan)) {
-			ast_log(LOG_ERROR, "error starting T.38 gateway for T.38 channel %s and G.711 channel %s\n", ast_channel_name(active), ast_channel_name(other));
+			ast_log(LOG_ERROR, "error starting T.38 gateway for T.38 channel %s and G.711 channel %s\n", ast_channel_name(other), ast_channel_name(active));
 			gateway->t38_state = T38_STATE_REJECTED;
+			details->is_t38_negotiated = 0;
 			control_params->request_response = AST_T38_REFUSED;
 
 			ast_framehook_detach(chan, details->gateway_id);
 			details->gateway_id = -1;
 		} else {
 			gateway->t38_state = T38_STATE_NEGOTIATED;
+			details->is_t38_negotiated = chan == other;
 			control_params->request_response = AST_T38_NEGOTIATED;
 		}
 
@@ -3209,6 +3231,7 @@
 		details->gateway_id = -1;
 
 		gateway->t38_state = T38_STATE_REJECTED;
+		details->is_t38_negotiated = 0;
 		control_params->request_response = AST_T38_TERMINATED;
 
 		fax_gateway_indicate_t38(chan, active, control_params);
@@ -3224,6 +3247,7 @@
 		ast_string_field_set(details, result, "SUCCESS");
 		ast_string_field_set(details, resultstr, "no gateway necessary");
 		ast_string_field_set(details, error, "NATIVE_T38");
+		details->is_t38_negotiated = 1;
 		set_channel_variables(chan, details);
 
 		ao2_ref(details, -1);
@@ -3351,6 +3375,7 @@
 			ast_string_field_set(details, result, "FAILED");
 			ast_string_field_set(details, resultstr, "neither channel supports T.38");
 			ast_string_field_set(details, error, "T38_NEG_ERROR");
+			details->is_t38_negotiated = 0;
 			set_channel_variables(chan, details);
 			return f;
 		}
@@ -3395,6 +3420,7 @@
 			ast_string_field_set(details, result, "FAILED");
 			ast_string_field_build(details, resultstr, "no fax activity after %d ms", details->gateway_timeout);
 			ast_string_field_set(details, error, "TIMEOUT");
+			details->is_t38_negotiated = 0;
 			set_channel_variables(chan, details);
 			return f;
 		}
@@ -3525,6 +3551,7 @@
 		ast_string_field_set(details, result, "FAILED");
 		ast_string_field_set(details, resultstr, "error initializing gateway session");
 		ast_string_field_set(details, error, "INIT_ERROR");
+		details->is_t38_negotiated = 0;
 		set_channel_variables(chan, details);
 		report_fax_status(chan, details, "No Available Resource");
 		return -1;
@@ -3540,6 +3567,7 @@
 		ast_string_field_set(details, result, "FAILED");
 		ast_string_field_set(details, resultstr, "error attaching gateway to channel");
 		ast_string_field_set(details, error, "INIT_ERROR");
+		details->is_t38_negotiated = 0;
 		set_channel_variables(chan, details);
 		return -1;
 	}

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id525747254b64c1efe8b1b5973d52ff9719c2ae1
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>



More information about the asterisk-code-review mailing list