Fix race condition in futex-based QSemaphore

Add one and reset the wakeAll bit atomically.

This avoids a race in a case where an acquiring thread
is owning the semaphore, and deleting it after a
set number of releases (one for each thread referencing the semaphore).

Two releasing threads could enter the if statement under
futexNeedsWake(prevValue), the counter been incremented at
this point and reached the value being acquired, meaning the thread
acquiring can be awakened by just one of the two releasers,
delete the semaphore, and then the second releaser would access
the now deleted semaphore.

The patch avoids that by unsetting and reading the wakeAll bit
atomically, so only one thread will try to wake all threads.

Fixes: QTBUG-102484
Change-Id: I32172ed44d74378c627918e19b9e1aaadb5c6d1d
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
(cherry picked from commit 830b1550de303dd59c29a87c28e44fa41112b8f4)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

1 files changed, 6 insertions, 3 deletions

diff --git a/src/corelib/thread/qsemaphore.cpp b/src/corelib/thread/qsemaphore.cpp
index cc5540c098..60a76a2b4e 100644
--- a/src/corelib/thread/qsemaphore.cpp
+++ b/src/corelib/thread/qsemaphore.cpp
@@ -356,7 +356,12 @@ void QSemaphore::release(int n)
         quintptr nn = unsigned(n);
         if (futexHasWaiterCount)
             nn |= quint64(nn) << 32;    // token count replicated in high word
-        quintptr prevValue = u.fetchAndAddRelease(nn);
+        quintptr prevValue = u.loadRelaxed();
+        quintptr newValue;
+        do { // loop just to ensure the operations are done atomically
+            newValue = prevValue + nn;
+            newValue &= (futexNeedsWakeAllBit - 1);
+        } while (!u.testAndSetRelease(prevValue, newValue, prevValue));
         if (futexNeedsWake(prevValue)) {
 #ifdef FUTEX_OP
             if (futexHasWaiterCount) {
@@ -378,7 +383,6 @@ void QSemaphore::release(int n)
                 quint32 oparg = 0;
                 quint32 cmp = FUTEX_OP_CMP_NE;
                 quint32 cmparg = 0;
-                u.fetchAndAndRelease(futexNeedsWakeAllBit - 1);
                 futexWakeOp(*futexLow32(&u), n, INT_MAX, *futexHigh32(&u), FUTEX_OP(op, oparg, cmp, cmparg));
                 return;
             }
@@ -390,7 +394,6 @@ void QSemaphore::release(int n)
             //    its acquisition anyway, so it has to wait;
             // 2) it did not see the new counter value, in which case its
             //    futexWait will fail.
-            u.fetchAndAndRelease(futexNeedsWakeAllBit - 1);
             if (futexHasWaiterCount) {
                 futexWakeAll(*futexLow32(&u));
                 futexWakeAll(*futexHigh32(&u));