[Asterisk-code-review] chan sip.c: Fix deadlock potential in fax redirection. (asterisk[master])

[Anonymous Coward]

Anonymous Coward #1000019 has submitted this change and it was merged.

Change subject: chan_sip.c: Fix deadlock potential in fax redirection.
......................................................................


chan_sip.c: Fix deadlock potential in fax redirection.

The sip_read() has the potential to deadlock if an incoming fax happens
during the Playback or similar application.

* Fixed the potential deadlock by not calling ast_async_goto() with the
channel lock held.

* Made always eat the fax detection frame whether there is a fax extension
or not.

ASTERISK-26216
Reported by: Richard Mudgett

Change-Id: I6d3f5cccd4b77c3aa6ffc1a54c0f6bde61c9278e
---
M channels/chan_sip.c
1 file changed, 9 insertions(+), 7 deletions(-)

Approvals:
  Anonymous Coward #1000019: Verified
  Joshua Colp: Looks good to me, approved
  Corey Farrell: Looks good to me, but someone else must approve



diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index b2522b6..ab54c76 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -8609,29 +8609,31 @@
 	if (faxdetected && ast_test_flag(&p->flags[1], SIP_PAGE2_FAX_DETECT_CNG)) {
 		if (strcmp(ast_channel_exten(ast), "fax")) {
 			const char *target_context = S_OR(ast_channel_macrocontext(ast), ast_channel_context(ast));
-			/* We need to unlock 'ast' here because
+			/*
+			 * We need to unlock 'ast' here because
 			 * ast_exists_extension has the potential to start and
 			 * stop an autoservice on the channel. Such action is
 			 * prone to deadlock if the channel is locked.
+			 *
+			 * ast_async_goto() has its own restriction on not holding
+			 * the channel lock.
 			 */
 			sip_pvt_unlock(p);
 			ast_channel_unlock(ast);
+			ast_frfree(fr);
+			fr = &ast_null_frame;
 			if (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))) {
-				ast_channel_lock(ast);
-				sip_pvt_lock(p);
 				ast_verb(2, "Redirecting '%s' to fax extension due to CNG detection\n", 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_NOTICE, "Failed to async goto '%s' into fax of '%s'\n", ast_channel_name(ast), target_context);
 				}
-				ast_frfree(fr);
-				fr = &ast_null_frame;
 			} else {
-				ast_channel_lock(ast);
-				sip_pvt_lock(p);
 				ast_log(LOG_NOTICE, "FAX CNG detected but no fax extension\n");
 			}
+			ast_channel_lock(ast);
+			sip_pvt_lock(p);
 		}
 	}
 

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I6d3f5cccd4b77c3aa6ffc1a54c0f6bde61c9278e
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>