From 46d9b07deece6e0d5feed6fd2799ca0af7cd35ee Mon Sep 17 00:00:00 2001 From: David Faure Date: Wed, 9 Apr 2025 14:24:04 +0200 Subject: [PATCH] QPointer: fix data race on ExternalRefCountData The creation of the ExternalRefCountData was published with testAndSetOrdered() but the loadRelaxed() would only load the pointer value, not the effects of the constructor. Pick-to: 6.5 Fixes: QTBUG-135640 Change-Id: I3acbc51e763e8a291be3f7036e0d9cd3965a2ce8 Reviewed-by: Thiago Macieira Reviewed-by: Giuseppe D'Angelo (cherry picked from commit 253f34082f526ff1ffd9eaefac73cc9aa616ab2a) Reviewed-by: Qt Cherry-pick Bot (cherry picked from commit df454f51f884004c4e65386b690c565d60af3630) --- src/corelib/tools/qsharedpointer.cpp | 2 +- .../corelib/kernel/qpointer/tst_qpointer.cpp | 37 +++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/corelib/tools/qsharedpointer.cpp b/src/corelib/tools/qsharedpointer.cpp index 47634c2e74d..32e22869ad2 100644 --- a/src/corelib/tools/qsharedpointer.cpp +++ b/src/corelib/tools/qsharedpointer.cpp @@ -1444,7 +1444,7 @@ QtSharedPointer::ExternalRefCountData *QtSharedPointer::ExternalRefCountData::ge QObjectPrivate *d = QObjectPrivate::get(const_cast(obj)); Q_ASSERT_X(!d->wasDeleted, "QWeakPointer", "Detected QWeakPointer creation in a QObject being deleted"); - ExternalRefCountData *that = d->sharedRefcount.loadRelaxed(); + ExternalRefCountData *that = d->sharedRefcount.loadAcquire(); if (that) { that->weakref.ref(); return that; diff --git a/tests/auto/corelib/kernel/qpointer/tst_qpointer.cpp b/tests/auto/corelib/kernel/qpointer/tst_qpointer.cpp index fa95fe85f7b..7968aa6fcb4 100644 --- a/tests/auto/corelib/kernel/qpointer/tst_qpointer.cpp +++ b/tests/auto/corelib/kernel/qpointer/tst_qpointer.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include @@ -34,6 +35,7 @@ private slots: void disconnect(); void castDuringDestruction(); void threadSafety(); + void raceCondition(); void qvariantCast(); void constPointer(); @@ -529,6 +531,41 @@ void tst_QPointer::threadSafety() owner.wait(); } +void tst_QPointer::raceCondition() +{ + const int NUM_THREADS = 20; + const int ITERATIONS_PER_THREAD = 10; + + QSemaphore startSemaphore; + + QObject targetObject; + + std::vector> threads; + threads.reserve(NUM_THREADS); + + for (int i = 0; i < NUM_THREADS; ++i) { + QThread *thread = + QThread::create([&startSemaphore, &targetObject, ITERATIONS_PER_THREAD]() { + startSemaphore.acquire(); + + for (int j = 0; j < ITERATIONS_PER_THREAD; ++j) { + QPointer pointer(&targetObject); + Q_UNUSED(pointer); + } + }); + + threads.emplace_back(thread); + thread->start(); + } + + QTest::qWait(100); + startSemaphore.release(NUM_THREADS); + + for (const auto &thread : threads) { + QVERIFY(thread->wait(30000)); + } +} + void tst_QPointer::qvariantCast() { QFile file;