[Asterisk-code-review] chan_pjsip: Relock correct channel during "fax" redirect. (...asterisk[master])

[Friendly Automation]

Friendly Automation has submitted this change and it was merged. ( https://gerrit.asterisk.org/c/asterisk/+/12878 )

Change subject: chan_pjsip: Relock correct channel during "fax" redirect.
......................................................................

chan_pjsip: Relock correct channel during "fax" redirect.

When fax detection occurs on an outbound PJSIP channel the
redirect operation will result in a masquerade occurring and
the underlying channel on the session changing. The code
incorrectly relocked the new channel instead of the old
channel when returning. This resulted in the new channel
being locked indefinitely. The code now always acts on the
expected channel.

ASTERISK-28538

Change-Id: I2b2e60d07e74383ae7e90d752c036c4b02d6b3a3
---
M channels/chan_pjsip.c
1 file changed, 24 insertions(+), 14 deletions(-)

Approvals:
  Benjamin Keith Ford: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, approved
  Friendly Automation: Approved for Submit



diff --git a/channels/chan_pjsip.c b/channels/chan_pjsip.c
index 0035a02..31b6a6e 100644
--- a/channels/chan_pjsip.c
+++ b/channels/chan_pjsip.c
@@ -749,7 +749,8 @@
 }
 
 /*! \brief Internal helper function called when CNG tone is detected */
-static struct ast_frame *chan_pjsip_cng_tone_detected(struct ast_sip_session *session, struct ast_frame *f)
+static struct ast_frame *chan_pjsip_cng_tone_detected(struct ast_channel *ast, struct ast_sip_session *session,
+	struct ast_frame *f)
 {
 	const char *target_context;
 	int exists;
@@ -765,11 +766,11 @@
 	}
 
 	/* If already executing in the fax extension don't do anything */
-	if (!strcmp(ast_channel_exten(session->channel), "fax")) {
+	if (!strcmp(ast_channel_exten(ast), "fax")) {
 		return f;
 	}
 
-	target_context = S_OR(ast_channel_macrocontext(session->channel), ast_channel_context(session->channel));
+	target_context = S_OR(ast_channel_macrocontext(ast), ast_channel_context(ast));
 
 	/*
 	 * We need to unlock the channel here because ast_exists_extension has the
@@ -778,25 +779,30 @@
 	 *
 	 * ast_async_goto() has its own restriction on not holding the channel lock.
 	 */
-	ast_channel_unlock(session->channel);
+	ast_channel_unlock(ast);
 	ast_frfree(f);
 	f = &ast_null_frame;
-	exists = ast_exists_extension(session->channel, target_context, "fax", 1,
-		S_COR(ast_channel_caller(session->channel)->id.number.valid,
-			ast_channel_caller(session->channel)->id.number.str, NULL));
+	exists = ast_exists_extension(ast, target_context, "fax", 1,
+		S_COR(ast_channel_caller(ast)->id.number.valid,
+			ast_channel_caller(ast)->id.number.str, NULL));
 	if (exists) {
 		ast_verb(2, "Redirecting '%s' to fax extension due to CNG detection\n",
-			ast_channel_name(session->channel));
-		pbx_builtin_setvar_helper(session->channel, "FAXEXTEN", ast_channel_exten(session->channel));
-		if (ast_async_goto(session->channel, target_context, "fax", 1)) {
+			ast_channel_name(ast));
+		pbx_builtin_setvar_helper(ast, "FAXEXTEN", ast_channel_exten(ast));
+		if (ast_async_goto(ast, target_context, "fax", 1)) {
 			ast_log(LOG_ERROR, "Failed to async goto '%s' into fax extension in '%s'\n",
-				ast_channel_name(session->channel), target_context);
+				ast_channel_name(ast), target_context);
 		}
 	} else {
 		ast_log(LOG_NOTICE, "FAX CNG detected on '%s' but no fax extension in '%s'\n",
-			ast_channel_name(session->channel), target_context);
+			ast_channel_name(ast), target_context);
 	}
-	ast_channel_lock(session->channel);
+
+	/* It's possible for a masquerade to have occurred when doing the ast_async_goto resulting in
+	 * the channel on the session having changed. Since we need to return with the original channel
+	 * locked we lock the channel that was passed in and not session->channel.
+	 */
+	ast_channel_lock(ast);
 
 	return f;
 }
@@ -895,7 +901,11 @@
 			if (f->subclass.integer == 'f') {
 				ast_debug(3, "Channel driver fax CNG detected on %s\n",
 					ast_channel_name(ast));
-				f = chan_pjsip_cng_tone_detected(session, f);
+				f = chan_pjsip_cng_tone_detected(ast, session, f);
+				/* When chan_pjsip_cng_tone_detected returns it is possible for the
+				 * channel pointed to by ast and by session->channel to differ due to a
+				 * masquerade. It's best not to touch things after this.
+				 */
 			} else {
 				ast_debug(3, "* Detected inband DTMF '%c' on '%s'\n", f->subclass.integer,
 					ast_channel_name(ast));

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/12878
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I2b2e60d07e74383ae7e90d752c036c4b02d6b3a3
Gerrit-Change-Number: 12878
Gerrit-PatchSet: 1
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190918/430e6641/attachment-0001.html>