From 276ffc372e8e7f2023b81e72699724eb603280f3 Mon Sep 17 00:00:00 2001 From: Sigmund Augdal Date: Mon, 12 Dec 2011 09:46:04 +0100 Subject: [PATCH] Replaced unsafe recursive mutex implementation with safe version already available in pthreads. Without this change running dbus_signal_get and dbus_signal_send in different threads would cause stack corruptions, double unrefs of the shared connection (and consequent abort() from dbus-libs) and dead locks, and possibly other ill effects --- dbus/dbus-sysdeps-pthread.c | 134 +++++------------------------------------- 1 files changed, 16 insertions(+), 118 deletions(-) diff --git a/dbus/dbus-sysdeps-pthread.c b/dbus/dbus-sysdeps-pthread.c index 46e4204..7611eec 100644 --- a/dbus/dbus-sysdeps-pthread.c +++ b/dbus/dbus-sysdeps-pthread.c @@ -43,20 +43,11 @@ static dbus_bool_t have_monotonic_clock = 0; typedef struct { - pthread_mutex_t lock; /**< lock protecting count field */ - volatile int count; /**< count of how many times lock holder has recursively locked */ - volatile pthread_t holder; /**< holder of the lock if count >0, - valid but arbitrary thread if count - has ever been >0, uninitialized memory - if count has never been >0 */ -} DBusMutexPThread; - -typedef struct { pthread_cond_t cond; /**< the condition */ } DBusCondVarPThread; #define DBUS_MUTEX(m) ((DBusMutex*) m) -#define DBUS_MUTEX_PTHREAD(m) ((DBusMutexPThread*) m) +#define DBUS_MUTEX_PTHREAD(m) ((pthread_mutex_t*) m) #define DBUS_COND_VAR(c) ((DBusCondVar*) c) #define DBUS_COND_VAR_PTHREAD(c) ((DBusCondVarPThread*) c) @@ -79,14 +70,18 @@ typedef struct { static DBusMutex* _dbus_pthread_mutex_new (void) { - DBusMutexPThread *pmutex; + pthread_mutex_t *pmutex; int result; + pthread_mutexattr_t attr; + pthread_mutexattr_init(&attr); + pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); - pmutex = dbus_new (DBusMutexPThread, 1); + pmutex = dbus_new (pthread_mutex_t, 1); if (pmutex == NULL) return NULL; - result = pthread_mutex_init (&pmutex->lock, NULL); + result = pthread_mutex_init (pmutex, &attr); + pthread_mutexattr_destroy(&attr); if (result == ENOMEM || result == EAGAIN) { @@ -98,97 +93,19 @@ _dbus_pthread_mutex_new (void) PTHREAD_CHECK ("pthread_mutex_init", result); } - /* Only written */ - pmutex->count = 0; - - /* There's no portable way to have a "null" pthread afaik so we - * can't set pmutex->holder to anything sensible. We only access it - * once the lock is held (which means we've set it). - */ - return DBUS_MUTEX (pmutex); } static void _dbus_pthread_mutex_free (DBusMutex *mutex) { - DBusMutexPThread *pmutex = DBUS_MUTEX_PTHREAD (mutex); + pthread_mutex_t *pmutex = DBUS_MUTEX_PTHREAD (mutex); - _dbus_assert (pmutex->count == 0); - - PTHREAD_CHECK ("pthread_mutex_destroy", pthread_mutex_destroy (&pmutex->lock)); + PTHREAD_CHECK ("pthread_mutex_destroy", pthread_mutex_destroy (pmutex)); dbus_free (pmutex); } -static void -_dbus_pthread_mutex_lock (DBusMutex *mutex) -{ - DBusMutexPThread *pmutex = DBUS_MUTEX_PTHREAD (mutex); - pthread_t self = pthread_self (); - - /* If the count is > 0 then someone had the lock, maybe us. If it is - * 0, then it might immediately change right after we read it, - * but it will be changed by another thread; i.e. if we read 0, - * we assume that this thread doesn't have the lock. - * - * Not 100% sure this is safe, but ... seems like it should be. - */ - if (pmutex->count == 0) - { - /* We know we don't have the lock; someone may have the lock. */ - - PTHREAD_CHECK ("pthread_mutex_lock", pthread_mutex_lock (&pmutex->lock)); - - /* We now have the lock. Count must be 0 since it must be 0 when - * the lock is released by another thread, and we just now got - * the lock. - */ - _dbus_assert (pmutex->count == 0); - - pmutex->holder = self; - pmutex->count = 1; - } - else - { - /* We know someone had the lock, possibly us. Thus - * pmutex->holder is not pointing to junk, though it may not be - * the lock holder anymore if the lock holder is not us. If the - * lock holder is us, then we definitely have the lock. - */ - - if (pthread_equal (pmutex->holder, self)) - { - /* We already have the lock. */ - _dbus_assert (pmutex->count > 0); - } - else - { - /* Wait for the lock */ - PTHREAD_CHECK ("pthread_mutex_lock", pthread_mutex_lock (&pmutex->lock)); - pmutex->holder = self; - _dbus_assert (pmutex->count == 0); - } - - pmutex->count += 1; - } -} - -static void -_dbus_pthread_mutex_unlock (DBusMutex *mutex) -{ - DBusMutexPThread *pmutex = DBUS_MUTEX_PTHREAD (mutex); - - _dbus_assert (pmutex->count > 0); - - pmutex->count -= 1; - - if (pmutex->count == 0) - PTHREAD_CHECK ("pthread_mutex_unlock", pthread_mutex_unlock (&pmutex->lock)); - - /* We leave pmutex->holder set to ourselves, its content is undefined if count is 0 */ -} - static DBusCondVar * _dbus_pthread_condvar_new (void) { @@ -236,19 +153,10 @@ static void _dbus_pthread_condvar_wait (DBusCondVar *cond, DBusMutex *mutex) { - DBusMutexPThread *pmutex = DBUS_MUTEX_PTHREAD (mutex); + pthread_mutex_t *pmutex = DBUS_MUTEX_PTHREAD (mutex); DBusCondVarPThread *pcond = DBUS_COND_VAR_PTHREAD (cond); - int old_count; - _dbus_assert (pmutex->count > 0); - _dbus_assert (pthread_equal (pmutex->holder, pthread_self ())); - - old_count = pmutex->count; - pmutex->count = 0; /* allow other threads to lock */ - PTHREAD_CHECK ("pthread_cond_wait", pthread_cond_wait (&pcond->cond, &pmutex->lock)); - _dbus_assert (pmutex->count == 0); - pmutex->count = old_count; - pmutex->holder = pthread_self(); /* other threads may have locked the mutex in the meantime */ + PTHREAD_CHECK ("pthread_cond_wait", pthread_cond_wait (&pcond->cond, pmutex)); } static dbus_bool_t @@ -256,15 +164,11 @@ _dbus_pthread_condvar_wait_timeout (DBusCondVar *cond, DBusMutex *mutex, int timeout_milliseconds) { - DBusMutexPThread *pmutex = DBUS_MUTEX_PTHREAD (mutex); + pthread_mutex_t *pmutex = DBUS_MUTEX_PTHREAD (mutex); DBusCondVarPThread *pcond = DBUS_COND_VAR_PTHREAD (cond); struct timeval time_now; struct timespec end_time; int result; - int old_count; - - _dbus_assert (pmutex->count > 0); - _dbus_assert (pthread_equal (pmutex->holder, pthread_self ())); #ifdef HAVE_MONOTONIC_CLOCK if (have_monotonic_clock) @@ -287,19 +191,13 @@ _dbus_pthread_condvar_wait_timeout (DBusCondVar *cond, end_time.tv_nsec -= 1000*1000*1000; } - old_count = pmutex->count; - pmutex->count = 0; - result = pthread_cond_timedwait (&pcond->cond, &pmutex->lock, &end_time); + result = pthread_cond_timedwait (&pcond->cond, pmutex, &end_time); if (result != ETIMEDOUT) { PTHREAD_CHECK ("pthread_cond_timedwait", result); } - _dbus_assert (pmutex->count == 0); - pmutex->count = old_count; - pmutex->holder = pthread_self(); /* other threads may have locked the mutex in the meantime */ - /* return true if we did not time out */ return result != ETIMEDOUT; } @@ -341,8 +239,8 @@ static const DBusThreadFunctions pthread_functions = _dbus_pthread_condvar_wake_all, _dbus_pthread_mutex_new, _dbus_pthread_mutex_free, - _dbus_pthread_mutex_lock, - _dbus_pthread_mutex_unlock + (void(*)(DBusMutex *))pthread_mutex_lock, + (void(*)(DBusMutex *))pthread_mutex_unlock }; static void -- 1.7.6.4