Attachment 54357 Details for Bug 43744 – patch against dbus-1.2 branch for a fix of this problem

[patch] patch against dbus-1.2 branch for a fix of this problem

0001-Replaced-unsafe-recursive-mutex-implementation-with-.patch (text/plain), 7.73 KB, created by sigmund.augdal on 2011-12-12 03:52:41 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: sigmund.augdal

Created: 2011-12-12 03:52:41 UTC

Size: 7.73 KB

patch

obsolete

>From 276ffc372e8e7f2023b81e72699724eb603280f3 Mon Sep 17 00:00:00 2001
>From: Sigmund Augdal <sigmund.augdal@sonowand.com>
>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
>

Actions: View | Splinter Review

Attachments on bug 43744: 54357 | 57049 | 57050 | 57051 | 57052 | 57078 | 57079 | 57166 | 57212 | 57326 | 57327 | 57328 | 57329 | 57330 | 57331 | 57332 | 57761 | 57762

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.