[Development] QBasicMutex::lockInternal() race condition?

[Olivier Goffart]

On Thursday 20 September 2012 15:30:32 Tony Van Eerd wrote:
> I've been looking at the implementation of QMutex (et al), via woboq, ie
> http://code.woboq.org/qt5/qtbase/src/corelib/thread/qmutex.cpp.html.  (nice
> work on the code viewer, by the way!)

Thanks :-)

> 
> I'm wondering if there isn't a subtle race condition in lockInternal() at
> line 358:
> 
> 358                 if (!d->ref())
> 359                     continue; //that QMutexData was already released
> 
> In the case where you pick up the d_ptr that was allocated by another thread
> - how do you know that d is still valid?
> 
> IIUC, d comes from the freelist, so it is probably never actually an invalid
> pointer, but how do you know that:
> 
> 1 this mutex was locked by another thread
> 2 this mutex  was unlocked by that (or yet another) thread
> 3 the d_ptr was released
> 4 the released pointer was recycled into another mutex via the freelist
> 5 then line 358 runs.
> 
> ie d->ref() passes, but 'd' is for the wrong mutex?

We check for that case shortly after:

361	  if (d != d_ptr.loadAcquire()) {
362	     //Either the mutex is already unlocked, or relocked with another mutex
363	     d->deref();
364	     continue;
365	  }

If we referenced the wrong mutex, we will unreference it and continue.


I have a change to document more the internal, but it is not merged yet.
https://codereview.qt-project.org/33739


-- 
Olivier

Woboq - Qt services and support - http://woboq.com