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."
This commit is contained in:
Robert Osfield 2009-03-12 10:12:42 +00:00
parent 1211fd1120
commit 5a537261a6

View File

@ -32,14 +32,15 @@ class ReentrantMutex : public OpenThreads::Mutex
virtual int lock() virtual int lock()
{ {
if (_threadHoldingMutex==OpenThreads::Thread::CurrentThread() && _lockCount>0)
{ {
OpenThreads::ScopedLock<OpenThreads::Mutex> lock(_lockCountMutex); OpenThreads::ScopedLock<OpenThreads::Mutex> lock(_lockCountMutex);
if (_threadHoldingMutex==OpenThreads::Thread::CurrentThread() && _lockCount>0)
{
++_lockCount; ++_lockCount;
return 0; return 0;
} }
else }
{
int result = Mutex::lock(); int result = Mutex::lock();
if (result==0) if (result==0)
{ {
@ -50,7 +51,6 @@ class ReentrantMutex : public OpenThreads::Mutex
} }
return result; return result;
} }
}
virtual int unlock() virtual int unlock()
@ -87,14 +87,15 @@ class ReentrantMutex : public OpenThreads::Mutex
virtual int trylock() virtual int trylock()
{ {
if (_threadHoldingMutex==OpenThreads::Thread::CurrentThread() && _lockCount>0)
{ {
OpenThreads::ScopedLock<OpenThreads::Mutex> lock(_lockCountMutex); OpenThreads::ScopedLock<OpenThreads::Mutex> lock(_lockCountMutex);
if (_threadHoldingMutex==OpenThreads::Thread::CurrentThread() && _lockCount>0)
{
++_lockCount; ++_lockCount;
return 0; return 0;
} }
else }
{
int result = Mutex::trylock(); int result = Mutex::trylock();
if (result==0) if (result==0)
{ {
@ -105,7 +106,6 @@ class ReentrantMutex : public OpenThreads::Mutex
} }
return result; return result;
} }
}
private: private: