From f1adc51ca4d03fd7bcf55996588a11d00471698f Mon Sep 17 00:00:00 2001
From: Marc Mutz <marc.mutz@qt.io>
Date: Wed, 21 Jun 2023 10:38:00 +0200
Subject: [PATCH] QFutureSynchronizer: fix aliasing problem in setFuture()

When setFuture() was handed an element of m_futures, it would hold the
reference to past the clear(), which invalidates said reference.

Fix by taking the future by value instead of by cref.

While append() is not affected, as QList::append() already guards
against aliasing, do the same change there, both for consistency as
well as to optimize the common case of passing rvalues. It also means
we can use the rvalue overload of QList::append(), skipping the alias
analysis in the lvalue QList::append().

[ChangeLog][QtConcurrent][QFutureSynchronizer] Fixed a crash in
setFuture() if the argument was already a member of
QFutureSynchronizer::futures().

Change-Id: Ic0b212b9f265a746df9a6beb6272a5415d131442
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
(cherry picked from commit e8dcbaaaf61ee2164db61e70a5d61d7a5b849371)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
---
 src/corelib/thread/qfuturesynchronizer.h      | 12 +++++-----
 src/corelib/thread/qfuturesynchronizer.qdoc   |  6 ++---
 .../tst_qfuturesynchronizer.cpp               | 23 +++++++++++++++++++
 3 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/src/corelib/thread/qfuturesynchronizer.h b/src/corelib/thread/qfuturesynchronizer.h
index 2cf2ca1b760..183e3ab3424 100644
--- a/src/corelib/thread/qfuturesynchronizer.h
+++ b/src/corelib/thread/qfuturesynchronizer.h
@@ -18,21 +18,21 @@ class QFutureSynchronizer
 
 public:
     QFutureSynchronizer() : m_cancelOnWait(false) { }
-    explicit QFutureSynchronizer(const QFuture<T> &future)
+    explicit QFutureSynchronizer(QFuture<T> future)
         : m_cancelOnWait(false)
-    { addFuture(future); }
+    { addFuture(std::move(future)); }
     ~QFutureSynchronizer()  { waitForFinished(); }
 
-    void setFuture(const QFuture<T> &future)
+    void setFuture(QFuture<T> future)
     {
         waitForFinished();
         m_futures.clear();
-        addFuture(future);
+        addFuture(std::move(future));
     }
 
-    void addFuture(const QFuture<T> &future)
+    void addFuture(QFuture<T> future)
     {
-        m_futures.append(future);
+        m_futures.append(std::move(future));
     }
 
     void waitForFinished()
diff --git a/src/corelib/thread/qfuturesynchronizer.qdoc b/src/corelib/thread/qfuturesynchronizer.qdoc
index 8dd7d58c4ae..7d2d16d6e96 100644
--- a/src/corelib/thread/qfuturesynchronizer.qdoc
+++ b/src/corelib/thread/qfuturesynchronizer.qdoc
@@ -38,7 +38,7 @@
 */
 
 /*!
-    \fn template <typename T> QFutureSynchronizer<T>::QFutureSynchronizer(const QFuture<T> &future)
+    \fn template <typename T> QFutureSynchronizer<T>::QFutureSynchronizer(QFuture<T> future)
 
     Constructs a QFutureSynchronizer and begins watching \a future by calling
     addFuture().
@@ -56,7 +56,7 @@
 */
 
 /*!
-    \fn template <typename T> void QFutureSynchronizer<T>::setFuture(const QFuture<T> &future)
+    \fn template <typename T> void QFutureSynchronizer<T>::setFuture(QFuture<T> future)
 
     Sets \a future to be the only future managed by this QFutureSynchronizer.
     This is a convenience function that calls waitForFinished(),
@@ -66,7 +66,7 @@
 */
 
 /*!
-    \fn template <typename T> void QFutureSynchronizer<T>::addFuture(const QFuture<T> &future)
+    \fn template <typename T> void QFutureSynchronizer<T>::addFuture(QFuture<T> future)
 
     Adds \a future to the list of managed futures.
 
diff --git a/tests/auto/corelib/thread/qfuturesynchronizer/tst_qfuturesynchronizer.cpp b/tests/auto/corelib/thread/qfuturesynchronizer/tst_qfuturesynchronizer.cpp
index fe84ec1ae40..b2efd6473b6 100644
--- a/tests/auto/corelib/thread/qfuturesynchronizer/tst_qfuturesynchronizer.cpp
+++ b/tests/auto/corelib/thread/qfuturesynchronizer/tst_qfuturesynchronizer.cpp
@@ -13,6 +13,7 @@ class tst_QFutureSynchronizer : public QObject
 
 private Q_SLOTS:
     void construction();
+    void setFutureAliasingExistingMember();
     void addFuture();
     void cancelOnWait();
     void clearFutures();
@@ -33,6 +34,28 @@ void tst_QFutureSynchronizer::construction()
     QCOMPARE(synchronizerWithFuture.futures().size(), 1);
 }
 
+void tst_QFutureSynchronizer::setFutureAliasingExistingMember()
+{
+    //
+    // GIVEN: a QFutureSynchronizer with one QFuture:
+    //
+    QFutureSynchronizer synchronizer(QtFuture::makeReadyValueFuture(42));
+
+    //
+    // WHEN: calling setFuture() with an alias of the QFuture already in `synchronizer`:
+    //
+    for (int i = 0; i < 2; ++i) {
+        const auto &f = synchronizer.futures().constFirst();
+        synchronizer.setFuture(f);
+    }
+
+    //
+    // THEN: it didn't crash
+    //
+    QCOMPARE(synchronizer.futures().size(), 1);
+    QCOMPARE(synchronizer.futures().constFirst().result(), 42);
+}
+
 void tst_QFutureSynchronizer::addFuture()
 {
     QFutureSynchronizer<void> synchronizer;