[Asterisk-code-review] pbx: deadlock when outgoing dialed channel hangs up too quickly (...asterisk[13])

[Friendly Automation]

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

Change subject: pbx: deadlock when outgoing dialed channel hangs up too quickly
......................................................................

pbx: deadlock when outgoing dialed channel hangs up too quickly

Here's the basic scenario that occurred when executing an AMI fast originate
while at the same time something else locks the channels container, and also
wants a lock on the dialed channel:

1. pbx_outgoing_attempt obtains a lock on a dialed channel
2. concurrently another thread obtains a lock on the channels container, and
   subsequently requests a lock on the dialed channel. It waits on #1. For
   instance, "core show channel <dialed channel"
3. the outgoing call does not fail, but ends before the pbx_outgoing_attempt
   function exits
4. pbx_outgoing_attempt function exits, the outgoing structure destructs, and
   attempts to hang up the dialed channel
5. hang up tries to obtain the channels container lock, but can't due to #2.
6. Asterisk is deadlocked.

The solution was to allow the pbx_outgoing_exec function to "steal" ownership
of the dialed channel, and handle hanging it up. The channel now is either hung
up prior to it being potentially locked by the initiating thread, or if locked
the hang up takes place in a different thread, thus alleviating the deadlock.

ASTERISK-28561
patches:
  iliketrains.diff submitted by Joshua Colp (license 5000)

Change-Id: I51b42b92dde8f2215b69bb509e28667ee3a3853a
---
M main/pbx.c
1 file changed, 14 insertions(+), 12 deletions(-)

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



diff --git a/main/pbx.c b/main/pbx.c
index d4ba2d2..6584735 100644
--- a/main/pbx.c
+++ b/main/pbx.c
@@ -7520,6 +7520,7 @@
 {
 	RAII_VAR(struct pbx_outgoing *, outgoing, data, ao2_cleanup);
 	enum ast_dial_result res;
+	struct ast_channel *chan;
 
 	res = ast_dial_run(outgoing->dial, NULL, 0);
 
@@ -7540,36 +7541,37 @@
 		return NULL;
 	}
 
+	/* We steal the channel so we get ownership of when it is hung up */
+	chan = ast_dial_answered_steal(outgoing->dial);
+
 	if (!ast_strlen_zero(outgoing->app)) {
 		struct ast_app *app = pbx_findapp(outgoing->app);
 
 		if (app) {
 			ast_verb(4, "Launching %s(%s) on %s\n", outgoing->app, S_OR(outgoing->appdata, ""),
-				ast_channel_name(ast_dial_answered(outgoing->dial)));
-			pbx_exec(ast_dial_answered(outgoing->dial), app, outgoing->appdata);
+				ast_channel_name(chan));
+			pbx_exec(chan, app, outgoing->appdata);
 		} else {
 			ast_log(LOG_WARNING, "No such application '%s'\n", outgoing->app);
 		}
-	} else {
-		struct ast_channel *answered = ast_dial_answered(outgoing->dial);
 
+		ast_hangup(chan);
+	} else {
 		if (!ast_strlen_zero(outgoing->context)) {
-			ast_channel_context_set(answered, outgoing->context);
+			ast_channel_context_set(chan, outgoing->context);
 		}
 
 		if (!ast_strlen_zero(outgoing->exten)) {
-			ast_channel_exten_set(answered, outgoing->exten);
+			ast_channel_exten_set(chan, outgoing->exten);
 		}
 
 		if (outgoing->priority > 0) {
-			ast_channel_priority_set(answered, outgoing->priority);
+			ast_channel_priority_set(chan, outgoing->priority);
 		}
 
-		if (ast_pbx_run(answered)) {
-			ast_log(LOG_ERROR, "Failed to start PBX on %s\n", ast_channel_name(answered));
-		} else {
-			/* PBX will have taken care of hanging up, so we steal the answered channel so dial doesn't do it */
-			ast_dial_answered_steal(outgoing->dial);
+		if (ast_pbx_run(chan)) {
+			ast_log(LOG_ERROR, "Failed to start PBX on %s\n", ast_channel_name(chan));
+			ast_hangup(chan);
 		}
 	}
 

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

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: I51b42b92dde8f2215b69bb509e28667ee3a3853a
Gerrit-Change-Number: 13032
Gerrit-PatchSet: 2
Gerrit-Owner: Kevin Harwell <kharwell 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-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20191014/9e3ed5f3/attachment.html>