From 4672843ae05739b7a1d99ed69063826b9830bc46 Mon Sep 17 00:00:00 2001 From: Ahmed Essam Date: Sat, 3 Jun 2023 00:49:25 +0300 Subject: [PATCH] Fix segfault when using qfuture continuations with move only types When using move-only types, continuations args are set using takeResult function, which has the side effect of invalidating the QFutureInterface associated with the promise/futures by: 1. setting isValid to false 2. setting the state to NoState And when the promise is destroyed, it tries to run the continuations if `finished()` is not called, which is done by checking the Finished bit in the state. But since the continuation has been run before, and the state has been set to NoState it tries to run the continuation again causing a segfault. Multiple solutions come in mind: 1. don't run the continuation if the state is NoState, but this would break the case when an empty promise is destroyed 2. check inside the continuation if it has been run before, and if so don't run it again, but this seems hacky since we don't want the continuation to be run twice, and it should break if it did. 3. when invalidating the promise leave the state as is, and change isValid only to false, which changes the current behavior, but is still compatible with the documentation which states only that isValid will return false if takeResult is called I chose option 3 I also extended some tests to test for move only types, and added a test that continuations run when a promise is finished. This simple case would segfault before with move only types. Fixes: QTBUG-112513 Change-Id: Ie225ac4fdf618e4edfb0efd663d6c7fd6b916dbd Reviewed-by: Ivan Solovev (cherry picked from commit 1f22fc995a36193cd67e8190858bb33614d149f4) Reviewed-by: Thiago Macieira --- src/corelib/thread/qfutureinterface.cpp | 1 - .../corelib/thread/qfuture/tst_qfuture.cpp | 11 --- .../corelib/thread/qpromise/tst_qpromise.cpp | 88 ++++++++++++++++--- 3 files changed, 77 insertions(+), 23 deletions(-) diff --git a/src/corelib/thread/qfutureinterface.cpp b/src/corelib/thread/qfutureinterface.cpp index 55565514475..0e39cd6da3e 100644 --- a/src/corelib/thread/qfutureinterface.cpp +++ b/src/corelib/thread/qfutureinterface.cpp @@ -619,7 +619,6 @@ void QFutureInterfaceBase::reset() { d->m_progressValue = 0; d->m_progress.reset(); - d->setState(QFutureInterfaceBase::NoState); d->progressTime.invalidate(); d->isValid = false; } diff --git a/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp b/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp index 79ca38209e2..19a67af1e42 100644 --- a/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp +++ b/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp @@ -3432,17 +3432,6 @@ void tst_QFuture::testFutureTaken(QFuture &noMoreFuture) { QCOMPARE(noMoreFuture.isValid(), false); QCOMPARE(noMoreFuture.resultCount(), 0); - QCOMPARE(noMoreFuture.isStarted(), false); - QCOMPARE(noMoreFuture.isRunning(), false); - QCOMPARE(noMoreFuture.isSuspending(), false); - QCOMPARE(noMoreFuture.isSuspended(), false); -#if QT_DEPRECATED_SINCE(6, 0) -QT_WARNING_PUSH -QT_WARNING_DISABLE_DEPRECATED - QCOMPARE(noMoreFuture.isPaused(), false); -QT_WARNING_POP -#endif - QCOMPARE(noMoreFuture.isFinished(), false); QCOMPARE(noMoreFuture.progressValue(), 0); } diff --git a/tests/auto/corelib/thread/qpromise/tst_qpromise.cpp b/tests/auto/corelib/thread/qpromise/tst_qpromise.cpp index 0b0e6c321f9..fc1ace10cbb 100644 --- a/tests/auto/corelib/thread/qpromise/tst_qpromise.cpp +++ b/tests/auto/corelib/thread/qpromise/tst_qpromise.cpp @@ -44,6 +44,7 @@ private slots: void cancelWhenReassigned(); void cancelWhenDestroyedWithoutStarting(); void cancelWhenDestroyedRunsContinuations(); + void continuationsRunWhenFinished(); void finishWhenSwapped(); void cancelWhenMoved(); void waitUntilResumed(); @@ -290,6 +291,10 @@ void tst_QPromise::setException() std::make_exception_ptr(TestException())); RUN_TEST_FUNC(testExceptionCaught, QPromise(), std::make_exception_ptr(TestException())); + RUN_TEST_FUNC(testExceptionCaught, QPromise(), + std::make_exception_ptr(TestException())); + RUN_TEST_FUNC(testExceptionCaught, QPromise(), + std::make_exception_ptr(TestException())); } #endif @@ -303,6 +308,8 @@ void tst_QPromise::cancel() testCancel(QPromise()); testCancel(QPromise()); + testCancel(QPromise()); + testCancel(QPromise()); } void tst_QPromise::progress() @@ -330,6 +337,8 @@ void tst_QPromise::progress() RUN_TEST_FUNC(testProgress, QPromise()); RUN_TEST_FUNC(testProgress, QPromise()); + RUN_TEST_FUNC(testProgress, QPromise()); + RUN_TEST_FUNC(testProgress, QPromise()); } void tst_QPromise::addInThread() @@ -455,6 +464,8 @@ void tst_QPromise::doNotCancelWhenFinished() RUN_TEST_FUNC(testFinishedPromise, QPromise()); RUN_TEST_FUNC(testFinishedPromise, QPromise()); RUN_TEST_FUNC(testFinishedPromise, QPromise()); + RUN_TEST_FUNC(testFinishedPromise, QPromise()); + RUN_TEST_FUNC(testFinishedPromise, QPromise()); #endif } @@ -516,11 +527,12 @@ void tst_QPromise::cancelWhenReassigned() #endif } -void tst_QPromise::cancelWhenDestroyedWithoutStarting() +template +static inline void testCancelWhenDestroyedWithoutStarting() { - QFuture future; + QFuture future; { - QPromise promise; + QPromise promise; future = promise.future(); } future.waitForFinished(); @@ -529,17 +541,26 @@ void tst_QPromise::cancelWhenDestroyedWithoutStarting() QVERIFY(future.isFinished()); } -void tst_QPromise::cancelWhenDestroyedRunsContinuations() +void tst_QPromise::cancelWhenDestroyedWithoutStarting() { - QFuture future; + testCancelWhenDestroyedWithoutStarting(); + testCancelWhenDestroyedWithoutStarting(); + testCancelWhenDestroyedWithoutStarting(); + testCancelWhenDestroyedWithoutStarting(); +} + +template +static inline void testCancelWhenDestroyedRunsContinuations() +{ + QFuture future; bool onCanceledCalled = false; bool thenCalled = false; { - QPromise promise; + QPromise promise; future = promise.future(); - future.then([&] { + future.then([&] (auto&&) { thenCalled = true; - }).onCanceled([&] { + }).onCanceled([&] () { onCanceledCalled = true; }); } @@ -548,6 +569,42 @@ void tst_QPromise::cancelWhenDestroyedRunsContinuations() QVERIFY(onCanceledCalled); } +void tst_QPromise::cancelWhenDestroyedRunsContinuations() +{ + testCancelWhenDestroyedRunsContinuations(); + testCancelWhenDestroyedRunsContinuations(); + testCancelWhenDestroyedRunsContinuations(); + testCancelWhenDestroyedRunsContinuations(); +} + +template +static inline void testContinuationsRunWhenFinished() +{ + QPromise promise; + QFuture future = promise.future(); + + bool thenCalled = false; + future.then([&] (auto&&) { + thenCalled = true; + }); + + promise.start(); + if constexpr (!std::is_void_v) { + promise.addResult(T{}); + } + promise.finish(); + + QVERIFY(thenCalled); +} + +void tst_QPromise::continuationsRunWhenFinished() +{ + testContinuationsRunWhenFinished(); + testContinuationsRunWhenFinished(); + testContinuationsRunWhenFinished(); + testContinuationsRunWhenFinished(); +} + void tst_QPromise::finishWhenSwapped() { #if !QT_CONFIG(cxx11_future) @@ -590,16 +647,17 @@ void tst_QPromise::finishWhenSwapped() #endif } -void tst_QPromise::cancelWhenMoved() +template +void testCancelWhenMoved() { #if !QT_CONFIG(cxx11_future) QSKIP("This test requires QThread::create"); #else - QPromise promise1; + QPromise promise1; auto f1 = promise1.future(); promise1.start(); - QPromise promise2; + QPromise promise2; auto f2 = promise2.future(); promise2.start(); @@ -623,6 +681,14 @@ void tst_QPromise::cancelWhenMoved() #endif } +void tst_QPromise::cancelWhenMoved() +{ + testCancelWhenMoved(); + testCancelWhenMoved(); + testCancelWhenMoved(); + testCancelWhenMoved(); +} + void tst_QPromise::waitUntilResumed() { #if !QT_CONFIG(cxx11_future)