[Asterisk-code-review] sla: Prevent deadlock and crash due to autoservicing. (asterisk[18])

[Friendly Automation]

Friendly Automation has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/19372 )

Change subject: sla: Prevent deadlock and crash due to autoservicing.
......................................................................

sla: Prevent deadlock and crash due to autoservicing.

SLAStation currently autoservices the station channel before
creating a thread to actually dial the trunk. This leads
to duplicate servicing of the channel which causes assertions,
deadlocks, crashes, and moreover not the correct behavior.

Removing the autoservice prevents the crash, but if the station
hangs up before the trunk answers, the call hangs since the hangup
was never serviced on the channel.

This is fixed by not autoservicing the channel, but instead
servicing it in the thread dialing the trunk, since it is doing
so synchronously to begin with. Instead of sleeping for 100ms
in a loop, we simply use the channel for timing, and abort
if it disappears.

The same issue also occurs with SLATrunk when a call is answered,
because ast_answer invokes ast_waitfor_nandfds. Thus, we use
ast_raw_answer instead which does not cause any conflict and allows
the call to be answered normally without thread blocking issues.

ASTERISK-29998 #close

Change-Id: Icc237d50354b5910000d2305901e86d2c87bb9d8
---
M apps/app_meetme.c
1 file changed, 48 insertions(+), 7 deletions(-)

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




diff --git a/apps/app_meetme.c b/apps/app_meetme.c
index 191de68..8a98fd9 100644
--- a/apps/app_meetme.c
+++ b/apps/app_meetme.c
@@ -6149,7 +6149,7 @@
 
 static void answer_trunk_chan(struct ast_channel *chan)
 {
-	ast_answer(chan);
+	ast_raw_answer(chan);
 	ast_indicate(chan, -1);
 }
 
@@ -6994,8 +6994,18 @@
 		return NULL;
 	}
 
-	for (;;) {
+	/* Wait for dial to end, while servicing the channel */
+	while (ast_waitfor(trunk_ref->chan, 100)) {
 		unsigned int done = 0;
+		struct ast_frame *fr = ast_read(trunk_ref->chan);
+
+		if (!fr) {
+			ast_debug(1, "Channel %s did not return a frame, must have hung up\n", ast_channel_name(trunk_ref->chan));
+			done = 1;
+			break;
+		}
+		ast_frfree(fr); /* Ignore while dialing */
+
 		switch ((dial_res = ast_dial_state(dial))) {
 		case AST_DIAL_RESULT_ANSWERED:
 			trunk_ref->trunk->chan = ast_dial_answered(dial);
@@ -7032,8 +7042,6 @@
 			last_state = current_state;
 		}
 
-		/* avoid tight loop... sleep for 1/10th second */
-		ast_safe_sleep(trunk_ref->chan, 100);
 	}
 
 	if (!trunk_ref->trunk->chan) {
@@ -7192,8 +7200,10 @@
 		sla_change_trunk_state(trunk_ref->trunk, SLA_TRUNK_STATE_UP, ALL_TRUNK_REFS, NULL);
 		/* Create a thread to dial the trunk and dump it into the conference.
 		 * However, we want to wait until the trunk has been dialed and the
-		 * conference is created before continuing on here. */
-		ast_autoservice_start(chan);
+		 * conference is created before continuing on here.
+		 * Don't autoservice the channel or we'll have multiple threads
+		 * handling it. dial_trunk services the channel.
+		 */
 		ast_mutex_init(&cond_lock);
 		ast_cond_init(&cond, NULL);
 		ast_mutex_lock(&cond_lock);
@@ -7202,7 +7212,7 @@
 		ast_mutex_unlock(&cond_lock);
 		ast_mutex_destroy(&cond_lock);
 		ast_cond_destroy(&cond);
-		ast_autoservice_stop(chan);
+
 		if (!trunk_ref->trunk->chan) {
 			ast_debug(1, "Trunk didn't get created. chan: %lx\n", (unsigned long) trunk_ref->trunk->chan);
 			pbx_builtin_setvar_helper(chan, "SLASTATION_STATUS", "CONGESTION");

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

Gerrit-Project: asterisk
Gerrit-Branch: 18
Gerrit-Change-Id: Icc237d50354b5910000d2305901e86d2c87bb9d8
Gerrit-Change-Number: 19372
Gerrit-PatchSet: 2
Gerrit-Owner: N A <asterisk at phreaknet.org>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20221128/6a5b79fb/attachment.html>