[svn-commits] mjordan: branch mjordan/trunk-deadlock r376021 - in /team/mjordan/trunk-deadl...

Wed Nov 7 13:21:54 CST 2012

Author: mjordan
Date: Wed Nov  7 13:21:50 2012
New Revision: 376021

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=376021
Log:
Create branch with some potential locking fixes

There are two weird locking scenarios that result in undefined behavior:

1) When creating a thread, we register it via ast_register_thread before
we initialize the mutex in the lock_info stored on the thread's
threadstorage.  However, the lock_info object will be obtained from
threadstorage when the thread registry's RWLOCK is manipulated during
ast_register_thread.  Since we interact with the mutex on the thread
(which will be a de-facto 'normal' pthead mutex) and then re-initialize
it as a recursive mutex, this effectively results in undefined behavior.

2) There is a race condition when creating the recursive mutex that
protects the lock information for a given mutex.  Two simultaneous attempts
to grab that mutex will start off with a NULL lock information object,
resulting in a race to initialize it.  This can result in two initializations
of the same mutex, which is also undefined.

This patch moves some stuff around and puts a global lock around the
recursive lock initialization.  No idea if this will help the goofy
lock freeze ups we see with DEBUG_THREADS enabled, but its a start.

Added:
    team/mjordan/trunk-deadlock/   (props changed)
      - copied from r375967, trunk/
Modified:
    team/mjordan/trunk-deadlock/include/asterisk/lock.h
    team/mjordan/trunk-deadlock/main/utils.c

Propchange: team/mjordan/trunk-deadlock/
------------------------------------------------------------------------------

--- branch-10-digiumphones-merged (added)
+++ branch-10-digiumphones-merged Wed Nov  7 13:21:50 2012
@@ -1,0 +1,1 @@
+/branches/10-digiumphones:364766,365396,368791,368963-368965,368999,369026

Propchange: team/mjordan/trunk-deadlock/
------------------------------------------------------------------------------
    branch-11-blocked = /branches/11:373240,375247,375702

Propchange: team/mjordan/trunk-deadlock/
------------------------------------------------------------------------------
Binary property 'branch-11-merged' - no diff available.

Propchange: team/mjordan/trunk-deadlock/
------------------------------------------------------------------------------
    certified-branch-1.8.11-merged = /certified/branches/1.8.11:364761,365395

Propchange: team/mjordan/trunk-deadlock/
------------------------------------------------------------------------------
--- reviewboard:url (added)
+++ reviewboard:url Wed Nov  7 13:21:50 2012
@@ -1,0 +1,1 @@
+https://reviewboard.asterisk.org

Propchange: team/mjordan/trunk-deadlock/
------------------------------------------------------------------------------
--- svn:externals (added)
+++ svn:externals Wed Nov  7 13:21:50 2012
@@ -1,0 +1,1 @@
+menuselect https://origsvn.digium.com/svn/menuselect/trunk

Propchange: team/mjordan/trunk-deadlock/
------------------------------------------------------------------------------
--- svn:ignore (added)
+++ svn:ignore Wed Nov  7 13:21:50 2012
@@ -1,0 +1,20 @@
+.applied
+.cleancount
+.depend
+.lastclean
+.tags-depend
+.tags-sources
+.version
+TAGS
+aclocal.m4
+autom4te.cache
+config.cache
+config.log
+config.status
+defaults.h
+makeopts
+makeopts.embed_rules
+menuselect-tree
+menuselect.makedeps
+menuselect.makeopts
+tags

Propchange: team/mjordan/trunk-deadlock/
------------------------------------------------------------------------------
    svn:mergeinfo = /branches/11:372696

Modified: team/mjordan/trunk-deadlock/include/asterisk/lock.h
URL: http://svnview.digium.com/svn/asterisk/team/mjordan/trunk-deadlock/include/asterisk/lock.h?view=diff&rev=376021&r1=375967&r2=376021
==============================================================================
--- team/mjordan/trunk-deadlock/include/asterisk/lock.h (original)
+++ team/mjordan/trunk-deadlock/include/asterisk/lock.h Wed Nov  7 13:21:50 2012
@@ -124,6 +124,8 @@
 	struct ast_lock_track *track;
 	unsigned int tracking:1;
 };
+
+static pthread_mutex_t reentrancy_lock = PTHREAD_MUTEX_INIT_VALUE;
 
 /*! \brief Structure for rwlock and tracking information.
  *
@@ -428,12 +430,20 @@
 
 static inline void ast_reentrancy_lock(struct ast_lock_track *lt)
 {
-	pthread_mutex_lock(&lt->reentr_mutex);
+	int res;
+	res = pthread_mutex_lock(&lt->reentr_mutex);
+	if (res) {
+		fprintf(stderr, "Failed to lock reentrancy mutex: %s[%d]\n", strerror(res), res);
+	}
 }
 
 static inline void ast_reentrancy_unlock(struct ast_lock_track *lt)
 {
-	pthread_mutex_unlock(&lt->reentr_mutex);
+	int res;
+	res = pthread_mutex_unlock(&lt->reentr_mutex);
+	if (res) {
+		fprintf(stderr, "Failed to unlock reentrancy mutex: %s[%d]\n", strerror(res), res);
+	}
 }
 
 static inline void ast_reentrancy_init(struct ast_lock_track **plt)
@@ -442,9 +452,14 @@
 	pthread_mutexattr_t reentr_attr;
 	struct ast_lock_track *lt = *plt;
 
+	pthread_mutex_lock(&reentrancy_lock);
 	if (!lt) {
 		lt = *plt = (struct ast_lock_track *) calloc(1, sizeof(*lt));
+	} else {
+		pthread_mutex_unlock(&reentrancy_lock);
+		goto bail;
 	}
+	pthread_mutex_unlock(&reentrancy_lock);
 
 	for (i = 0; i < AST_MAX_REENTRANCY; i++) {
 		lt->file[i] = NULL;
@@ -462,6 +477,8 @@
 	pthread_mutexattr_settype(&reentr_attr, AST_MUTEX_KIND);
 	pthread_mutex_init(&lt->reentr_mutex, &reentr_attr);
 	pthread_mutexattr_destroy(&reentr_attr);
+bail:
+	return;
 }
 
 static inline void delete_reentrancy_cs(struct ast_lock_track **plt)

Modified: team/mjordan/trunk-deadlock/main/utils.c
URL: http://svnview.digium.com/svn/asterisk/team/mjordan/trunk-deadlock/main/utils.c?view=diff&rev=376021&r1=375967&r2=376021
==============================================================================
--- team/mjordan/trunk-deadlock/main/utils.c (original)
+++ team/mjordan/trunk-deadlock/main/utils.c Wed Nov  7 13:21:50 2012
@@ -1001,6 +1001,23 @@
 	pthread_mutexattr_t mutex_attr;
 #endif
 
+#ifdef DEBUG_THREADS
+	if (!(lock_info = ast_threadstorage_get(&thread_lock_info, sizeof(*lock_info))))
+		return NULL;
+
+	lock_info->thread_id = pthread_self();
+	lock_info->thread_name = strdup(a.name);
+
+	pthread_mutexattr_init(&mutex_attr);
+	pthread_mutexattr_settype(&mutex_attr, AST_MUTEX_KIND);
+	pthread_mutex_init(&lock_info->lock, &mutex_attr);
+	pthread_mutexattr_destroy(&mutex_attr);
+
+	pthread_mutex_lock(&lock_infos_lock.mutex); /* Intentionally not the wrapper */
+	AST_LIST_INSERT_TAIL(&lock_infos, lock_info, entry);
+	pthread_mutex_unlock(&lock_infos_lock.mutex); /* Intentionally not the wrapper */
+#endif /* DEBUG_THREADS */
+
 	/* note that even though data->name is a pointer to allocated memory,
 	   we are not freeing it here because ast_register_thread is going to
 	   keep a copy of the pointer and then ast_unregister_thread will
@@ -1009,23 +1026,6 @@
 	ast_free(data);
 	ast_register_thread(a.name);
 	pthread_cleanup_push(ast_unregister_thread, (void *) pthread_self());
-
-#ifdef DEBUG_THREADS
-	if (!(lock_info = ast_threadstorage_get(&thread_lock_info, sizeof(*lock_info))))
-		return NULL;
-
-	lock_info->thread_id = pthread_self();
-	lock_info->thread_name = strdup(a.name);
-
-	pthread_mutexattr_init(&mutex_attr);
-	pthread_mutexattr_settype(&mutex_attr, AST_MUTEX_KIND);
-	pthread_mutex_init(&lock_info->lock, &mutex_attr);
-	pthread_mutexattr_destroy(&mutex_attr);
-
-	pthread_mutex_lock(&lock_infos_lock.mutex); /* Intentionally not the wrapper */
-	AST_LIST_INSERT_TAIL(&lock_infos, lock_info, entry);
-	pthread_mutex_unlock(&lock_infos_lock.mutex); /* Intentionally not the wrapper */
-#endif /* DEBUG_THREADS */
 
 	ret = a.start_routine(a.data);
 


