From 5a537261a6cc036ec8bbccc7fdc6181a32af46e3 Mon Sep 17 00:00:00 2001
From: Robert Osfield <robert@openscenegraph.com>
Date: Thu, 12 Mar 2009 10:12:42 +0000
Subject: [PATCH] From Michael Platings, "I've been looking at the discussion
 from 2006 ("[osg-users] osgDB/Reentrant Mutex not threadsafe ?") about this,
 and having looked closely at OpenThreads::ReentrantMutex it's still not
 thread safe in the following situation:

In my example case, there are 2 threads - one is a worker thread created by OpenThreads::Thread. The other thread is the main thread i.e. the thread that is intrinsically created when you execute the application. The crucial problem is that for the main thread, OpenThreads::Thread::CurrentThread() will return null.

I'll demonstrate this by breaking ReentrantMutex::lock() into sub-statements:

1.) if (_threadHoldingMutex==OpenThreads::Thread::CurrentThread())

2.) if (_lockCount>0){

3.)
OpenThreads::ScopedLock<OpenThreads::Mutex> lock(_lockCountMutex);
++_lockCount;
return 0;

4.)
int result = Mutex::lock();
if (result==0)
{
OpenThreads::ScopedLock<OpenThreads::Mutex> lock(_lockCountMutex);

5.)
_threadHoldingMutex = OpenThreads::Thread::CurrentThread();
_lockCount = 1;
return result;


An error will occur in the following case:
1) The worker thread calls lock(), it gets to the start of statement 5.
2) The main thread calls lock(). Statement 1 is evaluated as true as _threadHoldingMutex is null, and OpenThreads::Thread::CurrentThread() returns null.
3) The worker thread executes statement 5.
4) The main thread executes statement 2 and evaluates it as true, because the worker thread has set _lockCount to 1. The main thread executes statement 3, and now can access the mutexed-data at the same time as the worker thread!

The simple solution to this is to always protect access to _lockCount and _threadHoldingMutex using _lockCountMutex. I have done this in the file I am submitting."
---
 include/OpenThreads/ReentrantMutex | 52 +++++++++++++++---------------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/include/OpenThreads/ReentrantMutex b/include/OpenThreads/ReentrantMutex
index 8dae6a7aa..dc2f173aa 100644
--- a/include/OpenThreads/ReentrantMutex
+++ b/include/OpenThreads/ReentrantMutex
@@ -32,24 +32,24 @@ class ReentrantMutex : public OpenThreads::Mutex
 
         virtual int lock()
         {
-            if (_threadHoldingMutex==OpenThreads::Thread::CurrentThread() && _lockCount>0)
             {
                 OpenThreads::ScopedLock<OpenThreads::Mutex> lock(_lockCountMutex);
-                ++_lockCount;
-                return 0;
-            }
-            else
-            {
-                int result = Mutex::lock();
-                if (result==0)
+                if (_threadHoldingMutex==OpenThreads::Thread::CurrentThread() && _lockCount>0)
                 {
-                    OpenThreads::ScopedLock<OpenThreads::Mutex> lock(_lockCountMutex);
-
-                    _threadHoldingMutex = OpenThreads::Thread::CurrentThread();
-                    _lockCount = 1;
+                    ++_lockCount;
+                    return 0;
                 }
-                return result;
             }
+
+            int result = Mutex::lock();
+            if (result==0)
+            {
+                OpenThreads::ScopedLock<OpenThreads::Mutex> lock(_lockCountMutex);
+
+                _threadHoldingMutex = OpenThreads::Thread::CurrentThread();
+                _lockCount = 1;
+            }
+            return result;
         }
 
 
@@ -87,24 +87,24 @@ class ReentrantMutex : public OpenThreads::Mutex
 
         virtual int trylock()
         {
-            if (_threadHoldingMutex==OpenThreads::Thread::CurrentThread() && _lockCount>0)
             {
                 OpenThreads::ScopedLock<OpenThreads::Mutex> lock(_lockCountMutex);
-                ++_lockCount;
-                return 0;
-            }
-            else
-            {
-                int result = Mutex::trylock();
-                if (result==0)
+                if (_threadHoldingMutex==OpenThreads::Thread::CurrentThread() && _lockCount>0)
                 {
-                    OpenThreads::ScopedLock<OpenThreads::Mutex> lock(_lockCountMutex);
-
-                    _threadHoldingMutex = OpenThreads::Thread::CurrentThread();
-                    _lockCount = 1;
+                    ++_lockCount;
+                    return 0;
                 }
-                return result;
             }
+
+            int result = Mutex::trylock();
+            if (result==0)
+            {
+                OpenThreads::ScopedLock<OpenThreads::Mutex> lock(_lockCountMutex);
+
+                _threadHoldingMutex = OpenThreads::Thread::CurrentThread();
+                _lockCount = 1;
+            }
+            return result;
         }
 
     private: