[Asterisk-code-review] pbx: Deadlock between contexts container and context merge l... (asterisk[master])

[Kevin Harwell]

Kevin Harwell has uploaded a new change for review.

  https://gerrit.asterisk.org/1857

Change subject: pbx: Deadlock between contexts container and context_merge locks
......................................................................

pbx: Deadlock between contexts container and context_merge locks

Recent changes (ASTERISK-25394 commit 2bd27d12223fe33b58c453965ed5c6ed3af7c4f5)
introduced the possibility of a deadlock. Due to the mentioned modifications
ast_change_hints now needs to keep both merge/delete and state callbacks from
occurring while it executes. Unfortunately, sometimes ast_change_hints can be
called with the contexts container locked. When this happens it's possible for
another thread to grab the context_merge_lock before the thread calling into
ast_change_hints does and then try to obtain the contexts container lock. This
of course causes a deadlock between the two threads. The thread calling into
ast_change_hints waits for the other thread to release context_merge_lock and
the other thread is waiting on that one to release the contexts container lock.

Unfortunately, there is not a great way to fix this problem. When hints change
the merge/delete cannot run, nor the state callbacks. Picking the least of
evils this patch adds yet another lock: state_callbacks_lock. This lock is held
during state callbacks and ast_change_hints. In order to keep the merge/delete
code from executing at the same time ast_change_hints locks the contexts
container as well. The context_merge_lock no longer needs to be held during
ast_change_hints, thus relieving the possible tension between threads.

ASTERISK-25640 #close
Reported by: Krzysztof Trempala

Change-Id: I7f67ce4b4c94b731c23932cc2ba9e0a4a720813d
---
M main/pbx.c
1 file changed, 27 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/57/1857/1

diff --git a/main/pbx.c b/main/pbx.c
index 202940e..de0fac9 100644
--- a/main/pbx.c
+++ b/main/pbx.c
@@ -1496,6 +1496,11 @@
 AST_MUTEX_DEFINE_STATIC(context_merge_lock);
 
 /*!
+ * \brief Lock used to protect state data during callbacks
+ */
+AST_MUTEX_DEFINE_STATIC(state_callbacks_lock);
+
+/*!
  * \brief Registered applications container.
  *
  * It is sorted by application name.
@@ -5442,12 +5447,14 @@
 	strcpy(cmpdevice->hintdevice, dev_state->device);
 
 	ast_mutex_lock(&context_merge_lock);/* Hold off ast_merge_contexts_and_delete */
+	ast_mutex_lock(&state_callbacks_lock);
 	dev_iter = ao2_t_callback(hintdevices,
 		OBJ_SEARCH_OBJECT | OBJ_MULTIPLE,
 		hintdevice_cmp_multiple,
 		cmpdevice,
 		"find devices in container");
 	if (!dev_iter) {
+		ast_mutex_unlock(&state_callbacks_lock);
 		ast_mutex_unlock(&context_merge_lock);
 		ast_free(hint_app);
 		return;
@@ -5543,6 +5550,7 @@
 
 		ao2_cleanup(device_state_info);
 	}
+	ast_mutex_unlock(&state_callbacks_lock);
 	ast_mutex_unlock(&context_merge_lock);
 
 	ao2_iterator_destroy(dev_iter);
@@ -5946,7 +5954,14 @@
 		return -1;
 	}
 
-	ast_mutex_lock(&context_merge_lock); /* Hold off ast_merge_contexts_and_delete and state changes */
+	/* Lock state callbacks so hints can be manipulated */
+	ast_mutex_lock(&state_callbacks_lock);
+
+	/*
+	 * Hold off ast_merge_contexts_and_delete, but do it by holding the contexts lock.
+	 * If the context_merge_lock is held there is the potential for a deadlock.
+	 */
+	ast_rdlock_contexts();
 
 	ao2_lock(hints);/* Locked to hold off others while we move the hint around. */
 
@@ -5957,7 +5972,8 @@
 	hint = ao2_find(hints, oe, OBJ_UNLINK);
 	if (!hint) {
 		ao2_unlock(hints);
-		ast_mutex_unlock(&context_merge_lock);
+		ast_unlock_contexts();
+		ast_mutex_unlock(&state_callbacks_lock);
 		ast_free(hint_app);
 		return -1;
 	}
@@ -5998,9 +6014,10 @@
 
 	ao2_unlock(hints);
 
-	/* Locking for state callbacks is respected here and only the context_merge_lock lock is
-	 * held during the state callback invocation. This will stop the normal state callback
-	 * thread from being able to handle incoming state changes if they occur.
+	/* Locking for state callbacks is respected here and only the context_merge_lock and
+	 * state_callbacks_lock are held during the state callback invocation. This will stop
+	 * the normal state callback thread from being able to handle incoming state changes
+	 * if they occur.
 	 */
 
 	/* Determine if presence state has changed due to the change of the hint extension */
@@ -6083,7 +6100,8 @@
 
 	ao2_ref(hint, -1);
 
-	ast_mutex_unlock(&context_merge_lock);
+	ast_unlock_contexts();
+	ast_mutex_unlock(&state_callbacks_lock);
 
 	ast_free(hint_app);
 	ast_free(previous_message);
@@ -12111,12 +12129,14 @@
 	strcpy(cmpdevice->hintdevice, presence_state->provider);
 
 	ast_mutex_lock(&context_merge_lock);/* Hold off ast_merge_contexts_and_delete */
+	ast_mutex_lock(&state_callbacks_lock);
 	dev_iter = ao2_t_callback(hintdevices,
 		OBJ_POINTER | OBJ_MULTIPLE,
 		hintdevice_cmp_multiple,
 		cmpdevice,
 		"find devices in container");
 	if (!dev_iter) {
+		ast_mutex_unlock(&state_callbacks_lock);
 		ast_mutex_unlock(&context_merge_lock);
 		ast_free(hint_app);
 		return;
@@ -12193,6 +12213,7 @@
 		ao2_iterator_destroy(&cb_iter);
 	}
 	ao2_iterator_destroy(dev_iter);
+	ast_mutex_unlock(&state_callbacks_lock);
 	ast_mutex_unlock(&context_merge_lock);
 
 	ast_free(hint_app);

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I7f67ce4b4c94b731c23932cc2ba9e0a4a720813d
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>