QFuture: prevent the continuations from being executed twice

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 <marten.nordheim@qt.io>
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
(cherry picked from commit 04b5d2b94f96c73a13973f6a57cefbf07d2e850b)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
This commit is contained in:
Ivan Solovev 2025-02-07 14:55:11 +01:00 committed by Qt Cherry-pick Bot
parent 90b128e4cc
commit 5b55e8d1f0
2 changed files with 4 additions and 1 deletions

View File

@ -888,6 +888,7 @@ void QFutureInterfaceBase::setContinuation(std::function<void(const QFutureInter
// If the state is ready, run continuation immediately,
// otherwise save it for later.
if (isFinished()) {
d->continuationExecuted = 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);

View File

@ -162,6 +162,7 @@ public:
enum ContinuationState : quint8 { Default, Canceled, Cleaned };
std::atomic<ContinuationState> continuationState { Default };
bool continuationExecuted = false;
inline QThreadPool *pool() const
{ return m_pool ? m_pool : QThreadPool::globalInstance(); }