[XERCESC-1762] Tru64 PlatformUtils RecursiveMutex implementation is not thread-safe

• Type: Improvement
• Status: Closed
• Priority: Critical
• Resolution: Fixed
• Affects Version/s: 2.7.0, 2.8.0
• Component/s: Miscellaneous
• Environment: Tru64 5.1
• Assignee: Unassigned
• Reporter: Vladimir Lazarenko
• Created: 06/Dec/07 06:59
• Updated: 03/Nov/09 08:28
• Resolved: 03/Nov/09 08:28

Description

The way, how RecursiveMutex is implemented is not thread safe. We've come up with a different implementation and herewith would like to share it with you, with hope that it will make its way into the mainstream.

In essence the issue is that you can not check if you're the owner of the thread until you lock. If you don't lock, the tid can be overwritten, and the check will be bogus. Further on, gAtomicOpMutex could use PTHREAD_MUTEX_INITIALIZER instead of custom initialization.

-bash-2.05b$ svn diff -r21597 Tru64PlatformUtils.cpp
Index: Tru64PlatformUtils.cpp
===================================================================
— Tru64PlatformUtils.cpp (revision 21597)
+++ Tru64PlatformUtils.cpp (working copy)
@@ -42,7 +42,10 @@
#include <xercesc/util/XMLUniDefs.hpp>
#include <xercesc/util/PanicHandler.hpp>
#include <xercesc/util/OutOfMemoryException.hpp>
+#include <xercesc/util/XMemory.hpp>

+#include <assert.h>
+
//
// These control which transcoding service is used by the Tru64 version.
// They allow this to be controlled from the build process by just defining
@@ -397,9 +400,10 @@
// XMLPlatformUtils: Platform init method
// ---------------------------------------------------------------------------

-typedef XMLHolder<pthread_mutex_t> MutexHolderType;
+//typedef XMLHolder<pthread_mutex_t> MutexHolderType;

-static MutexHolderType* gAtomicOpMutex = 0;
+static pthread_mutex_t gAtomicOpMutex = PTHREAD_MUTEX_INITIALIZER;
+//static MutexHolderType* gAtomicOpMutex = 0;

void XMLPlatformUtils::platformInit()
{
@@ -411,14 +415,15 @@
// circular dependency between compareAndExchange() and
// mutex creation that must be broken.

+#if 0
gAtomicOpMutex = new (fgMemoryManager) MutexHolderType;

-
if (pthread_mutex_init(&gAtomicOpMutex->fInstance, NULL))

{ delete gAtomicOpMutex; gAtomicOpMutex = 0; panic( PanicHandler::Panic_SystemInit ); }

+#endif
}

@@ -427,61 +432,81 @@
// -----------------------------------------------------------------------

-class RecursiveMutex
+class RecursiveMutex : public XMemory
{
-public:

• pthread_mutex_t mutex;
• int recursionCount;
• pthread_t tid;
• MemoryManager* const fMemoryManager;
  +public :

• RecursiveMutex(MemoryManager* manager) :
• mutex(),
• recursionCount(0),
• tid(0),
• fMemoryManager(manager)
  + RecursiveMutex()
  + : mut(),
  + cond(),
  + count(0),
  + owner()
  {
• if (pthread_mutex_init(&mutex, NULL))
• XMLPlatformUtils::panic(PanicHandler::Panic_MutexErr);
  + int r = pthread_mutex_init(&mut, 0);
  + if (r != 0) { + XMLPlatformUtils::panic(PanicHandler::Panic_MutexErr); + }

  + r = pthread_cond_init(&cond, 0);
  + if (r != 0)

  { + pthread_mutex_destroy(&mut); + XMLPlatformUtils::panic(PanicHandler::Panic_MutexErr); + }

  }

• ~RecursiveMutex()
  + void lock()
  {
• if (pthread_mutex_destroy(&mutex))
• ThrowXMLwithMemMgr(XMLPlatformUtilsException, XMLExcepts::Mutex_CouldNotDestroy, fMemoryManager);
  + pthread_t self = pthread_self();
  +
  + int r = pthread_mutex_lock(&mut);
  + assert(r == 0);
  +
  + while (count != 0 && ! pthread_equal(self, owner)) { + r = pthread_cond_wait(&cond, &mut); + assert(r == 0); + }

  +
  + if (count++ == 0)

  { + owner = self; + }

  +
  + r = pthread_mutex_unlock(&mut);
  + assert(r == 0);
  }

• void lock()
  + void unlock()
  {
• if (pthread_equal(tid, pthread_self()))
• {
• recursionCount++;
• return;
  + int r = pthread_mutex_lock(&mut);
  + assert(r == 0);
  +
  + assert(pthread_equal(pthread_self(), owner));
  +
  + if (--count == 0) { + pthread_cond_signal(&cond); }
• if (pthread_mutex_lock(&mutex) != 0)
• XMLPlatformUtils::panic(PanicHandler::Panic_MutexErr);
• tid = pthread_self();
• recursionCount = 1;
  +
  + r = pthread_mutex_unlock(&mut);
  + assert(r == 0);
  }

-

• void unlock()
  + ~RecursiveMutex() { - if (--recursionCount > 0) - return; + assert(count == 0); + pthread_cond_destroy(&cond); + pthread_mutex_destroy(&mut); + }

• if (pthread_mutex_unlock(&mutex) != 0)
• XMLPlatformUtils::panic(PanicHandler::Panic_MutexErr);
• tid = 0;
• }
  +private :
  + pthread_mutex_t mut;
  + pthread_cond_t cond;
  + unsigned int count;
  + pthread_t owner; // valid if count != 0
  };

void* XMLPlatformUtils::makeMutex(MemoryManager* manager)

{ - return new (manager) RecursiveMutex(manager); + return new (manager) RecursiveMutex; }

-
void XMLPlatformUtils::closeMutex(void* const mtxHandle)
{
if (mtxHandle == NULL)
@@ -522,14 +547,14 @@
// the below calls are temporarily made till the above functions are part of user library
// Currently its supported only in the kernel mode

• if (pthread_mutex_lock( &gAtomicOpMutex->fInstance))
  + if (pthread_mutex_lock( &gAtomicOpMutex))
  panic(PanicHandler::Panic_SynchronizationErr);

void *retVal = *toFill;
if (*toFill == toCompare)
*toFill = (void *)newValue;

• if (pthread_mutex_unlock( &gAtomicOpMutex->fInstance))
  + if (pthread_mutex_unlock( &gAtomicOpMutex))
  panic(PanicHandler::Panic_SynchronizationErr);

return retVal;
@@ -539,12 +564,12 @@
{
//return (int)atomic_add_32_nv( (uint32_t*)&location, 1);

• if (pthread_mutex_lock( &gAtomicOpMutex->fInstance))
  + if (pthread_mutex_lock( &gAtomicOpMutex))
  panic(PanicHandler::Panic_SynchronizationErr);

int tmp = ++location;

• if (pthread_mutex_unlock( &gAtomicOpMutex->fInstance))
  + if (pthread_mutex_unlock( &gAtomicOpMutex))
  panic(PanicHandler::Panic_SynchronizationErr);

return tmp;
@@ -553,12 +578,12 @@
{
//return (int)atomic_add_32_nv( (uint32_t*)&location, -1);

• if (pthread_mutex_lock( &gAtomicOpMutex->fInstance))
  + if (pthread_mutex_lock( &gAtomicOpMutex))
  panic(PanicHandler::Panic_SynchronizationErr);

int tmp = --location;

• if (pthread_mutex_unlock( &gAtomicOpMutex->fInstance))
  + if (pthread_mutex_unlock( &gAtomicOpMutex))
  panic(PanicHandler::Panic_SynchronizationErr);

return tmp;
@@ -619,11 +644,6 @@

void XMLPlatformUtils::platformTerm()

{ -#if !defined (APP_NO_THREADS) - pthread_mutex_destroy(&gAtomicOpMutex->fInstance); - delete gAtomicOpMutex; - gAtomicOpMutex = 0; -#endif }

#include <xercesc/util/LogicalPath.c>