From 5371db4675cf92ce35e60f93628f6454ff9fead5 Mon Sep 17 00:00:00 2001 From: Allan Sandfeld Jensen Date: Tue, 12 Apr 2022 16:25:08 +0200 Subject: 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 (cherry picked from commit 830b1550de303dd59c29a87c28e44fa41112b8f4) Reviewed-by: Qt Cherry-pick Bot --- src/corelib/thread/qsemaphore.cpp | 9 ++++++--- 1 file 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)); -- cgit v1.2.3