[Asterisk-code-review] res fax: Fix latent bug exposed by ASTERISK-24841 changes. (asterisk[11])

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


Richard Mudgett has uploaded a new change for review.

  https://gerrit.asterisk.org/3700

Change subject: res_fax: Fix latent bug exposed by ASTERISK-24841 changes.
......................................................................

res_fax: Fix latent bug exposed by ASTERISK-24841 changes.

Three fax related tests started failing as a result of changes made for
ASTERISK-24841:
tests/fax/pjsip/gateway_t38_g711
tests/fax/sip/gateway_mix1
tests/fax/sip/gateway_mix3

Historically, ast_channel_make_compatible() did nothing if the channels
were already "compatible" even if they had a sub-optimal translation path
already setup.  With the changes from ASTERISK-24841 this is no longer
true in order to allow the best translation paths to always be picked.  In
res_fax.c:fax_gateway_framehook() code manually setup the channels to go
through slin and then called ast_channel_make_compatible().  With the
previous version of ast_channel_make_compatible() this was always a
no-operation.

* Remove call to ast_channel_make_compatible() in fax_gateway_framehook()
that now undoes what was just setup when the framehook is attached.

* Fixed locking around saving the channel formats in
fax_gateway_framehook() to ensure that the formats that are saved are
consistent.

* Fix copy pasta errors in fax_gateway_framehook() that confuses read and
write when dealing with saved channel formats.

ASTERISK-24841
Reported by: Matt Jordan

Backported from v13 revision 82bc0fd3ade77394e13062b6097732c224e77eaa
mainly to get the diff between v11 and v13 smaller but this also fixes
a potential deadlock when dealing with the chan and the peer together.

Change-Id: I6fda0877104a370af586a5e8cf9e161a484da78d
---
M res/res_fax.c
1 file changed, 26 insertions(+), 31 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/00/3700/1

diff --git a/res/res_fax.c b/res/res_fax.c
index 5f660fe..ad62f02 100644
--- a/res/res_fax.c
+++ b/res/res_fax.c
@@ -2959,8 +2959,13 @@
 static struct ast_frame *fax_gateway_framehook(struct ast_channel *chan, struct ast_frame *f, enum ast_framehook_event event, void *data)
 {
 	struct fax_gateway *gateway = data;
-	struct ast_channel *peer, *active;
-	struct ast_fax_session_details *details;
+	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;
@@ -2979,37 +2984,38 @@
 
 		if (gateway->bridged) {
 			ast_set_read_format(chan, &gateway->chan_read_format);
-			ast_set_read_format(chan, &gateway->chan_write_format);
+			ast_set_write_format(chan, &gateway->chan_write_format);
 
-			if ((peer = ast_bridged_channel(chan))) {
+			peer = ast_bridged_channel(chan);
+			if (peer) {
+				ao2_ref(peer, +1);
+				ast_channel_unlock(chan);
 				ast_set_read_format(peer, &gateway->peer_read_format);
-				ast_set_read_format(peer, &gateway->peer_write_format);
+				ast_set_write_format(peer, &gateway->peer_write_format);
 				ast_channel_make_compatible(chan, peer);
+				ast_channel_lock(chan);
 			}
 		}
-
-		ao2_ref(details, -1);
 		return NULL;
 	}
 
 	if (!f || (event == AST_FRAMEHOOK_EVENT_ATTACHED)) {
-		ao2_ref(details, -1);
 		return NULL;
 	};
 
 	/* this frame was generated by the fax gateway, pass it on */
 	if (ast_test_flag(f, AST_FAX_FRFLAG_GATEWAY)) {
-		ao2_ref(details, -1);
 		return f;
 	}
 
-	if (!(peer = ast_bridged_channel(chan))) {
-		/* not bridged, don't do anything */
-		ao2_ref(details, -1);
+	/* If we aren't bridged or we don't have a peer, don't do anything */
+	peer = ast_bridged_channel(chan);
+	if (!peer) {
 		return f;
 	}
+	ao2_ref(peer, +1);
 
-	if (!gateway->bridged && peer) {
+	if (!gateway->bridged) {
 		/* 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) {
 			ast_debug(1, "not starting gateway for %s and %s; neither channel supports T.38\n", ast_channel_name(chan), ast_channel_name(peer));
@@ -3020,7 +3026,6 @@
 			ast_string_field_set(details, resultstr, "neither channel supports T.38");
 			ast_string_field_set(details, error, "T38_NEG_ERROR");
 			set_channel_variables(chan, details);
-			ao2_ref(details, -1);
 			return f;
 		}
 
@@ -3028,13 +3033,16 @@
 			gateway->timeout_start = ast_tvnow();
 		}
 
+		ast_channel_unlock(chan);
+		ast_channel_lock_both(chan, peer);
+
 		/* we are bridged, change r/w formats to SLIN for v21 preamble
 		 * detection and T.30 */
 		ast_format_copy(&gateway->chan_read_format, ast_channel_readformat(chan));
-		ast_format_copy(&gateway->chan_write_format, ast_channel_readformat(chan));
+		ast_format_copy(&gateway->chan_write_format, ast_channel_writeformat(chan));
 
 		ast_format_copy(&gateway->peer_read_format, ast_channel_readformat(peer));
-		ast_format_copy(&gateway->peer_write_format, ast_channel_readformat(peer));
+		ast_format_copy(&gateway->peer_write_format, ast_channel_writeformat(peer));
 
 		ast_set_read_format_by_id(chan, AST_FORMAT_SLINEAR);
 		ast_set_write_format_by_id(chan, AST_FORMAT_SLINEAR);
@@ -3042,7 +3050,8 @@
 		ast_set_read_format_by_id(peer, AST_FORMAT_SLINEAR);
 		ast_set_write_format_by_id(peer, AST_FORMAT_SLINEAR);
 
-		ast_channel_make_compatible(chan, peer);
+		ast_channel_unlock(peer);
+
 		gateway->bridged = 1;
 		if (!(gateway->peer_v21_session = fax_v21_session_new(peer))) {
 			ast_log(LOG_ERROR, "Can't create V21 session on chan %s for T.38 gateway session\n", ast_channel_name(peer));
@@ -3061,7 +3070,6 @@
 			ast_string_field_build(details, resultstr, "no fax activity after %d ms", details->gateway_timeout);
 			ast_string_field_set(details, error, "TIMEOUT");
 			set_channel_variables(chan, details);
-			ao2_ref(details, -1);
 			return f;
 		}
 	}
@@ -3075,7 +3083,6 @@
 		case AST_FORMAT_ULAW:
 			break;
 		default:
-			ao2_ref(details, -1);
 			return f;
 		}
 		break;
@@ -3083,16 +3090,13 @@
 		if (f->subclass.integer == AST_MODEM_T38) {
 			break;
 		}
-		ao2_ref(details, -1);
 		return f;
 	case AST_FRAME_CONTROL:
 		if (f->subclass.integer == AST_CONTROL_T38_PARAMETERS) {
 			break;
 		}
-		ao2_ref(details, -1);
 		return f;
 	default:
-		ao2_ref(details, -1);
 		return f;
 	}
 
@@ -3106,20 +3110,17 @@
 		break;
 	default:
 		ast_log(LOG_WARNING, "unhandled framehook event %u\n", event);
-		ao2_ref(details, -1);
 		return f;
 	}
 
 	/* handle control frames */
 	if (f->frametype == AST_FRAME_CONTROL && f->subclass.integer == AST_CONTROL_T38_PARAMETERS) {
-		ao2_ref(details, -1);
 		return fax_gateway_detect_t38(gateway, chan, peer, active, f);
 	}
 
 	if (!gateway->detected_v21 && gateway->t38_state == T38_STATE_UNAVAILABLE && f->frametype == AST_FRAME_VOICE) {
 		/* not in gateway mode and have not detected v21 yet, listen
 		 * for v21 */
-		ao2_ref(details, -1);
 		return fax_gateway_detect_v21(gateway, chan, peer, active, f);
 	}
 
@@ -3132,7 +3133,6 @@
 			&& (readtrans = ast_channel_readtrans(active))) {
 			if ((f = ast_translate(readtrans, f, 1)) == NULL) {
 				f = &ast_null_frame;
-				ao2_ref(details, -1);
 				return f;
 			}
 			/* XXX we ignore the return value here, perhaps we should
@@ -3146,7 +3146,6 @@
 		}
 
 		f = &ast_null_frame;
-		ao2_ref(details, -1);
 		return f;
 	}
 
@@ -3162,16 +3161,12 @@
 			};
 			ast_format_set(&silence_frame.subclass.format, AST_FORMAT_SLINEAR, 0);
 			memset(silence_buf, 0, sizeof(silence_buf));
-
-			ao2_ref(details, -1);
 			return ast_frisolate(&silence_frame);
 		} else {
-			ao2_ref(details, -1);
 			return &ast_null_frame;
 		}
 	}
 
-	ao2_ref(details, -1);
 	return f;
 }
 

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

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



More information about the asterisk-code-review mailing list