From 5b55e8d1f066945dc36deda6285aebc3fd315e00 Mon Sep 17 00:00:00 2001 From: Ivan Solovev Date: Fri, 7 Feb 2025 14:55:11 +0100 Subject: [PATCH] QFuture: prevent the continuations from being executed twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On some very rare cases the continuation could be executed twice in a multithreaded scenario. Consider the following example: auto f = QtConcurrent::run(someFunction); f.then(someOtherFunction); Assuming that QtConcurrent::run() is executed on Thread B, and the continuation is attached in Thread A, the following call stack is possible: 1. Thread B: someFunction() is finished 2. Thread B: QFutureInterface::reportFinished() is called 3. Thread B: QFutureInterfaceBase::reportFinished() is called and finished. 4. Thread A: then() is called. It checks that the future is already finished, so the attached continuation is executed immediately. 5. Thread B: QFutureInterfaceBase::runContinuation() is executed. At this point it sees that a continuation exists, and executes it again. Step 5 will lead to a crash, because we'll try to access a moved-from QPromise. To fix the issue, introduce a flag that is explicitly used to test if the continuation was already executed or not. The pre-existing code already uses a concept of a continuation state, but it cannot be easily extended without convering it from an enum to flags, because the canceled continuation should still be called in order to actually execute the onCanceled() handler. Wrapping QFIBP::ContinuationState into QFlags would increase its size from 1 to 4 bytes, and manually treating the enum as flags will result in a more complicated code. So, I simply picked the approach of adding an explicit flag for this case. Writing a unit-test is not really possible, but I verified that the reproducer from the linked Jira ticket does not crash anymore. Amends dfaca09e85a49d2983bb89893bfbe1ba4c19eab4 that introduced the continuations, so picking to all active Qt 6 branches. Fixes: QTBUG-118032 Pick-to: 6.8 6.5 Change-Id: I3e07722495e38367e8e4be2eded34140e22b0053 Reviewed-by: Mårten Nordheim Reviewed-by: Fabian Kosmale (cherry picked from commit 04b5d2b94f96c73a13973f6a57cefbf07d2e850b) Reviewed-by: Qt Cherry-pick Bot --- src/corelib/thread/qfutureinterface.cpp | 4 +++- src/corelib/thread/qfutureinterface_p.h | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/corelib/thread/qfutureinterface.cpp b/src/corelib/thread/qfutureinterface.cpp index 7f1b2bef154..2236b5d1db1 100644 --- a/src/corelib/thread/qfutureinterface.cpp +++ b/src/corelib/thread/qfutureinterface.cpp @@ -888,6 +888,7 @@ void QFutureInterfaceBase::setContinuation(std::functioncontinuationExecuted = true; lock.unlock(); func(*this); lock.relock(); @@ -919,10 +920,11 @@ void QFutureInterfaceBase::cleanContinuation() void QFutureInterfaceBase::runContinuation() const { QMutexLocker lock(&d->continuationMutex); - if (d->continuation) { + if (d->continuation && !d->continuationExecuted) { // Save the continuation in a local function, to avoid calling // a null std::function below, in case cleanContinuation() is // called from some other thread right after unlock() below. + d->continuationExecuted = true; auto fn = std::move(d->continuation); lock.unlock(); fn(*this); diff --git a/src/corelib/thread/qfutureinterface_p.h b/src/corelib/thread/qfutureinterface_p.h index 92a10725910..c573ba8274d 100644 --- a/src/corelib/thread/qfutureinterface_p.h +++ b/src/corelib/thread/qfutureinterface_p.h @@ -162,6 +162,7 @@ public: enum ContinuationState : quint8 { Default, Canceled, Cleaned }; std::atomic continuationState { Default }; + bool continuationExecuted = false; inline QThreadPool *pool() const { return m_pool ? m_pool : QThreadPool::globalInstance(); }