[Asterisk-code-review] Fix deadlock on presence state changes. (asterisk[certified/13.1])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged.

Change subject: Fix deadlock on presence state changes.
......................................................................


Fix deadlock on presence state changes.

A deadlock was observed where three threads were competing for different
locks:

* One thread held the hints lock and was attempting to lock a specific
  hint.
* One thread was holding the specific hint's lock and was attempting to
  lock the contexts lock
* One thread was holding the contexts lock and attempting to lock the
  hints lock.

Clearly the second thread was doing the wrong thing here. The fix for
this is to make sure that the hint's lock is not held on presence state
changes. Something similar is already done (and commented about) for
device state changes.

ASTERISK-25362 #close
Reported by Mark Michelson

Change-Id: I15ec2416b92978a4c0c08273b2d46cb21aff97e2
---
M main/pbx.c
1 file changed, 14 insertions(+), 1 deletion(-)

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



diff --git a/main/pbx.c b/main/pbx.c
index 99f5db9..40008a1 100644
--- a/main/pbx.c
+++ b/main/pbx.c
@@ -11869,10 +11869,11 @@
 		struct ast_state_cb *state_cb;
 		const char *app;
 		char *parse;
-		SCOPED_AO2LOCK(lock, hint);
+		ao2_lock(hint);
 
 		if (!hint->exten) {
 			/* The extension has already been destroyed */
+			ao2_unlock(hint);
 			continue;
 		}
 
@@ -11880,16 +11881,19 @@
 		app = ast_get_extension_app(hint->exten);
 		if (ast_strlen_zero(app)) {
 			/* The hint does not monitor presence at all. */
+			ao2_unlock(hint);
 			continue;
 		}
 
 		ast_str_set(&hint_app, 0, "%s", app);
 		parse = parse_hint_presence(hint_app);
 		if (ast_strlen_zero(parse)) {
+			ao2_unlock(hint);
 			continue;
 		}
 		if (strcasecmp(parse, presence_state->provider)) {
 			/* The hint does not monitor the presence provider. */
+			ao2_unlock(hint);
 			continue;
 		}
 
@@ -11910,6 +11914,7 @@
 			((hint->last_presence_message && presence_state->message && !strcmp(hint->last_presence_message, presence_state->message)) || (!hint->last_presence_message && !presence_state->message))) {
 
 			/* this update is the same as the last, do nothing */
+			ao2_unlock(hint);
 			continue;
 		}
 
@@ -11919,6 +11924,14 @@
 		hint->last_presence_state = presence_state->state;
 		hint->last_presence_subtype = presence_state->subtype ? ast_strdup(presence_state->subtype) : NULL;
 		hint->last_presence_message = presence_state->message ? ast_strdup(presence_state->message) : NULL;
+		/*
+		 * (Copied from device_state_cb)
+		 *
+		 * NOTE: We cannot hold any locks while notifying
+		 * the watchers without causing a deadlock.
+		 * (conlock, hints, and hint)
+		 */
+		ao2_unlock(hint);
 
 		/* For general callbacks */
 		cb_iter = ao2_iterator_init(statecbs, 0);

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I15ec2416b92978a4c0c08273b2d46cb21aff97e2
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: certified/13.1
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>