[Asterisk-code-review] res fax: Fix deadlock in ast channel get t38 state(). (asterisk[14])

[Richard Mudgett]

Richard Mudgett has uploaded a new change for review.

  https://gerrit.asterisk.org/3717

Change subject: res_fax: Fix deadlock in ast_channel_get_t38_state().
......................................................................

res_fax: Fix deadlock in ast_channel_get_t38_state().

ast_channel_get_t38_state() calls ast_channel_queryoption() with
AST_OPTION_T38_STATE.  If the passed in channel is a local channel then a
deadlock can happen if a channel lock is held when called.

* Made ast_channel_get_t38_state() callers not hold a channel lock before
calling.

* Update ast_channel_get_t38_state() doxygen to note that no channel locks
can be held when calling the function.

ASTERISK-26203 #close
Reported by: Etienne Lessard

ASTERISK-24822 #close
Reported by: David Brillert

ASTERISK-22732 #close
Reported by: Richard Mudgett

Change-Id: I49fd76fa9af628b4198009b5c0b82c8b03681214
---
M include/asterisk/channel.h
M res/res_fax.c
2 files changed, 31 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/17/3717/1

diff --git a/include/asterisk/channel.h b/include/asterisk/channel.h
index 14bd32c..3293dd7 100644
--- a/include/asterisk/channel.h
+++ b/include/asterisk/channel.h
@@ -2549,7 +2549,11 @@
 	return 0;
 }
 
-/*! \brief Retrieves the current T38 state of a channel */
+/*!
+ * \brief Retrieves the current T38 state of a channel
+ *
+ * \note Absolutely _NO_ channel locks should be held before calling this function.
+ */
 static inline enum ast_t38_state ast_channel_get_t38_state(struct ast_channel *chan)
 {
 	enum ast_t38_state state = T38_STATE_UNAVAILABLE;
diff --git a/res/res_fax.c b/res/res_fax.c
index a369d68..a2e1793 100644
--- a/res/res_fax.c
+++ b/res/res_fax.c
@@ -3001,8 +3001,14 @@
 	}
 
 	if (gateway->detected_v21) {
+		enum ast_t38_state state_other;
+
 		destroy_v21_sessions(gateway);
-		if (ast_channel_get_t38_state(other) == T38_STATE_UNKNOWN) {
+
+		ast_channel_unlock(chan);
+		state_other = ast_channel_get_t38_state(other);
+		ast_channel_lock(chan);
+		if (state_other == T38_STATE_UNKNOWN) {
 			ast_debug(1, "detected v21 preamble from %s\n", ast_channel_name(active));
 			return fax_gateway_request_t38(gateway, chan, f);
 		} else {
@@ -3043,6 +3049,7 @@
 	struct ast_control_t38_parameters *control_params = f->data.ptr;
 	struct ast_channel *other = (active == chan) ? peer : chan;
 	struct ast_fax_session_details *details;
+	enum ast_t38_state state_other;
 
 	if (f->datalen != sizeof(struct ast_control_t38_parameters)) {
 		/* invalaid AST_CONTROL_T38_PARAMETERS frame, we can't
@@ -3065,9 +3072,11 @@
 	}
 
 	if (control_params->request_response == AST_T38_REQUEST_NEGOTIATE) {
-		enum ast_t38_state state = ast_channel_get_t38_state(other);
+		ast_channel_unlock(chan);
+		state_other = ast_channel_get_t38_state(other);
+		ast_channel_lock(chan);
 
-		if (state == T38_STATE_UNKNOWN) {
+		if (state_other == T38_STATE_UNKNOWN) {
 			/* we detected a request to negotiate T.38 and the
 			 * other channel appears to support T.38, we'll pass
 			 * the request through and only step in if the other
@@ -3080,7 +3089,7 @@
 			details->gateway_timeout = FAX_GATEWAY_TIMEOUT;
 			ao2_ref(details, -1);
 			return f;
-		} else if (state == T38_STATE_UNAVAILABLE || state == T38_STATE_REJECTED) {
+		} else if (state_other == T38_STATE_UNAVAILABLE || state_other == T38_STATE_REJECTED) {
 			/* the other channel does not support T.38, we need to
 			 * step in here */
 			ast_debug(1, "%s is attempting to negotiate T.38 but %s does not support it\n", ast_channel_name(active), ast_channel_name(other));
@@ -3160,7 +3169,10 @@
 		/* our request to negotiate T.38 was refused, if the other
 		 * channel supports T.38, they might still reinvite and save
 		 * the day.  Otherwise disable the gateway. */
-		if (ast_channel_get_t38_state(other) == T38_STATE_UNKNOWN) {
+		ast_channel_unlock(chan);
+		state_other = ast_channel_get_t38_state(other);
+		ast_channel_lock(chan);
+		if (state_other == T38_STATE_UNKNOWN) {
 			gateway->t38_state = T38_STATE_UNAVAILABLE;
 		} else {
 			ast_framehook_detach(chan, details->gateway_id);
@@ -3366,8 +3378,16 @@
 	}
 
 	if (!gateway->bridged) {
+		enum ast_t38_state state_chan;
+		enum ast_t38_state state_peer;
+
+		ast_channel_unlock(chan);
+		state_chan = ast_channel_get_t38_state(chan);
+		state_peer = ast_channel_get_t38_state(peer);
+		ast_channel_lock(chan);
+
 		/* don't start a gateway if neither channel can handle T.38 */
-		if (ast_channel_get_t38_state(chan) == T38_STATE_UNAVAILABLE && ast_channel_get_t38_state(peer) == T38_STATE_UNAVAILABLE) {
+		if (state_chan == T38_STATE_UNAVAILABLE && state_peer == T38_STATE_UNAVAILABLE) {
 			ast_debug(1, "not starting gateway for %s and %s; neither channel supports T.38\n", ast_channel_name(chan), ast_channel_name(peer));
 			ast_framehook_detach(chan, gateway->framehook);
 			details->gateway_id = -1;

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

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