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

[Richard Mudgett]

Richard Mudgett has uploaded a new change for review.

  https://gerrit.asterisk.org/3299

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

chan_dahdi.c: Fix deadlock potential in fax redirection.

The dahdi_handle_dtmf() and my_handle_dtmf() have 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.

ASTERISK-26216 #close
Reported by: Richard Mudgett

Change-Id: I9144b84ade5f96690996624ec8a2d40c56af40aa
---
M channels/chan_dahdi.c
1 file changed, 16 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/99/3299/1

diff --git a/channels/chan_dahdi.c b/channels/chan_dahdi.c
index e4b7c0e..308e093 100644
--- a/channels/chan_dahdi.c
+++ b/channels/chan_dahdi.c
@@ -1693,26 +1693,28 @@
 				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 ast_exists_extension has the
+					/*
+					 * We need to unlock 'ast' here because ast_exists_extension has the
 					 * potential to start autoservice on the channel. Such action is prone
-					 * to deadlock.
+					 * to deadlock if the channel is locked.
+					 *
+					 * ast_async_goto() has its own restriction on not holding the
+					 * channel lock.
 					 */
 					ast_mutex_unlock(&p->lock);
 					ast_channel_unlock(ast);
 					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);
-						ast_mutex_lock(&p->lock);
 						ast_verb(3, "Redirecting %s to fax extension\n", ast_channel_name(ast));
 						/* Save the DID/DNIS when we transfer the fax call to a "fax" extension */
 						pbx_builtin_setvar_helper(ast, "FAXEXTEN", ast_channel_exten(ast));
 						if (ast_async_goto(ast, target_context, "fax", 1))
 							ast_log(LOG_WARNING, "Failed to async goto '%s' into fax of '%s'\n", ast_channel_name(ast), target_context);
 					} else {
-						ast_channel_lock(ast);
-						ast_mutex_lock(&p->lock);
 						ast_log(LOG_NOTICE, "Fax detected, but no fax extension\n");
 					}
+					ast_channel_lock(ast);
+					ast_mutex_lock(&p->lock);
 				} else {
 					ast_debug(1, "Already in a fax extension, not redirecting\n");
 				}
@@ -7200,26 +7202,28 @@
 				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 ast_exists_extension has the
+					/*
+					 * We need to unlock 'ast' here because ast_exists_extension has the
 					 * potential to start autoservice on the channel. Such action is prone
-					 * to deadlock.
+					 * to deadlock if the channel is locked.
+					 *
+					 * ast_async_goto() has its own restriction on not holding the
+					 * channel lock.
 					 */
 					ast_mutex_unlock(&p->lock);
 					ast_channel_unlock(ast);
 					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);
-						ast_mutex_lock(&p->lock);
 						ast_verb(3, "Redirecting %s to fax extension\n", ast_channel_name(ast));
 						/* Save the DID/DNIS when we transfer the fax call to a "fax" extension */
 						pbx_builtin_setvar_helper(ast, "FAXEXTEN", ast_channel_exten(ast));
 						if (ast_async_goto(ast, target_context, "fax", 1))
 							ast_log(LOG_WARNING, "Failed to async goto '%s' into fax of '%s'\n", ast_channel_name(ast), target_context);
 					} else {
-						ast_channel_lock(ast);
-						ast_mutex_lock(&p->lock);
 						ast_log(LOG_NOTICE, "Fax detected, but no fax extension\n");
 					}
+					ast_channel_lock(ast);
+					ast_mutex_lock(&p->lock);
 				} else {
 					ast_debug(1, "Already in a fax extension, not redirecting\n");
 				}

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

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