From 529b648ece265f30784dadb96624e94b4ced5aba Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Sun, 28 Apr 2013 13:50:47 +0200 Subject: Fix possible race in QMutex As described in the QTBUG-30872, there may be a race condition involving 3 threads fighting for a mutex. I am surprised it was not caught before despite all the Q_ASSERT and the stress test in tst_qmutex. We do not need to call store(0) because the unlocking thread will eventually remove the BigNumber flag. And perhaps it even did it already, and another thread has incremented waiters (hence the Q_ASSERT is wrong) Here is a paste of part of the description from the bug report: --- many threads, one of them is ready to release mutex, while at least two other trying to acquire it d->waiters is 0 Thread 1 release mutex in unlockInternal: if (d->waiters.fetchAndAddRelease(-QMutexPrivate::BigNumber) == 0) d->waiters is now -QMutexPrivate::BigNumber Thread 2 try to acquire mutex in lockInternal: old_waiters = d->waiters.load(); if (old_waiters == -QMutexPrivate::BigNumber) { if (d_ptr.testAndSetAcquire(d, dummyLocked())) { It acquire 'about to release mutex' by changing d to dummyLocked Thread 1 continue release procedure: d->derefWaiters(0); d->waiters is now back to 0 Thread 3 try to acquire mutex in lockInternal: while (!d->waiters.testAndSetRelaxed(old_waiters, old_waiters + 1)); d->waiters is now 1 Thread 2 continue its dummy lock: d->waiters.store(0); d->waiters is force to 0 Thread 3 continue wait procedure but it realize that mutex was already unlocked so decrease back waiters if (d != d_ptr.loadAcquire()) { if (old_waiters != QMutexPrivate::BigNumber) { d->waiters.deref(); d->waiters became negative value of -1 Neither thread need internal data so it is released back to pool The waiters counter in released internal structure is still -1 --- Change-Id: I1b22555db764482775db6e64a8c9ffa9e1ab0cf6 Task-number: QTBUG-30872 Reviewed-by: Thiago Macieira --- src/corelib/thread/qmutex.cpp | 2 -- 1 file changed, 2 deletions(-) (limited to 'src/corelib') diff --git a/src/corelib/thread/qmutex.cpp b/src/corelib/thread/qmutex.cpp index 45e52d7b85..1ed4a77950 100644 --- a/src/corelib/thread/qmutex.cpp +++ b/src/corelib/thread/qmutex.cpp @@ -467,8 +467,6 @@ bool QBasicMutex::lockInternal(int timeout) QT_MUTEX_LOCK_NOEXCEPT // we try to acquire the mutex by changing to dummyLocked() if (d_ptr.testAndSetAcquire(d, dummyLocked())) { // Mutex acquired - Q_ASSERT(d->waiters.load() == -QMutexPrivate::BigNumber || d->waiters.load() == 0); - d->waiters.store(0); d->deref(); return true; } else { -- cgit v1.2.3