[Asterisk-code-review] stasis/control: Fix possible deadlock with swap channel (asterisk[master])

[George Joseph]

George Joseph has uploaded this change for review. ( https://gerrit.asterisk.org/6369


Change subject: stasis/control:  Fix possible deadlock with swap channel
......................................................................

stasis/control:  Fix possible deadlock with swap channel

If an error occurs during a bridge impart it's possible that
the "bridge_after" callback might try to run before
control_swap_channel_in_bridge has been signalled to continue.
Since control_swap_channel_in_bridge is holding the control lock
and the callback needs it, a deadlock will occur.

* control_swap_channel_in_bridge now only holds the control
  lock while it's actually modifying the control structure and
  releases it while the bridge impart is running.
* bridge_after_cb is now tolerant of impart failures.

Change-Id: Ifd239aa93955b3eb475521f61e284fcb0da2c3b3
---
M res/stasis/control.c
1 file changed, 21 insertions(+), 8 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/69/6369/1

diff --git a/res/stasis/control.c b/res/stasis/control.c
index 503f111..92b8776 100644
--- a/res/stasis/control.c
+++ b/res/stasis/control.c
@@ -989,8 +989,12 @@
 	SCOPED_AO2LOCK(lock, control);
 	struct ast_bridge_channel *bridge_channel;
 
+	/* control->bridge may be NULL because of a race with
+	 * control_swap_channel_in_bridge if an error occurrs
+	 * during the bridge impart.  We need to be tolerant of that.
+	 */
 	ast_debug(3, "%s, %s: Channel leaving bridge\n",
-		ast_channel_uniqueid(chan), control->bridge->uniqueid);
+		ast_channel_uniqueid(chan), control->bridge ? control->bridge->uniqueid : "FAILED");
 
 	ast_assert(chan == control->channel);
 
@@ -1000,16 +1004,19 @@
 
 	app_unsubscribe_bridge(control->app, control->bridge);
 
-	/* No longer in the bridge */
-	control->bridge = NULL;
-
 	/* Get the bridge channel so we don't depart from the wrong bridge */
 	ast_channel_lock(chan);
 	bridge_channel = ast_channel_get_bridge_channel(chan);
 	ast_channel_unlock(chan);
 
-	/* Depart this channel from the bridge using the command queue if possible */
-	stasis_app_send_command_async(control, bridge_channel_depart, bridge_channel, __ao2_cleanup);
+	if (control->bridge) {
+		/* Depart this channel from the bridge using the command queue if possible */
+		stasis_app_send_command_async(control, bridge_channel_depart, bridge_channel, __ao2_cleanup);
+	}
+
+	/* No longer in the bridge */
+	control->bridge = NULL;
+
 	if (stasis_app_channel_is_stasis_end_published(chan)) {
 		/* The channel has had a StasisEnd published on it, but until now had remained in
 		 * the bridging system. This means that the channel moved from a Stasis bridge to a
@@ -1191,25 +1198,31 @@
 			ast_channel_pbx_set(chan, NULL);
 		}
 
+		control->bridge = NULL;
+		/* We can't be holding the control lock while impart is running
+		 * or we could create a deadlock with bridge_after_cb which also
+		 * tries to lock control.
+		 */
+		ao2_unlock(control);
 		res = ast_bridge_impart(bridge,
 			chan,
 			swap,
 			NULL, /* features */
 			AST_BRIDGE_IMPART_CHAN_DEPARTABLE);
+		ao2_lock(control);
 		if (res != 0) {
 			ast_log(LOG_ERROR, "Error adding channel to bridge\n");
 			ast_channel_pbx_set(chan, control->pbx);
 			control->pbx = NULL;
 			return -1;
 		}
-
-		ast_assert(stasis_app_get_bridge(control) == NULL);
 		control->bridge = bridge;
 
 		ast_channel_lock(chan);
 		set_interval_hook(chan);
 		ast_channel_unlock(chan);
 	}
+
 	return 0;
 }
 

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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifd239aa93955b3eb475521f61e284fcb0da2c3b3
Gerrit-Change-Number: 6369
Gerrit-PatchSet: 1
Gerrit-Owner: George Joseph <gjoseph at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170901/f3a0cd28/attachment.html>