From b7c3eabdb8380be2a80ae4c688dfc743e7cebca1 Mon Sep 17 00:00:00 2001
From: Fabian Kosmale <fabian.kosmale@qt.io>
Date: Tue, 18 Apr 2023 08:54:36 +0200
Subject: [PATCH] Avoid capturing same property twice

Avoid capturing the same property multiple times in a binding by
storing them in the BindingEvaluationState. We store them in a
QVarLengthArray array, as the number of properties involved in a binding
is expected to be rather low, so a linear scan is fine.

Avoiding double capture is a good idea in general, as we would otherwise
needlessly reevaluate bindings multiple times, and also needlessly
allocate memory for further observers, instead of using a binding's
inline observer array.

Even more importantantly, our notification code makes assumptions that
notify will visit bindings only exactly once. Not upholding that
invariant leads to memory corruption and subsequent crashes, as
observers allocated by the binding would get freed, even though we would
still access them later.

Fixes: QTBUG-112822
Change-Id: Icdc1f43fe554df6fa69e881872b2c429d5fa0bbc
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
(cherry picked from commit cb30e45b9a800c6ad9cdfb446a20b6a6e8efbe71)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
---
 src/corelib/kernel/qproperty.cpp                |  5 +++++
 src/corelib/kernel/qproperty_p.h                |  1 +
 .../corelib/kernel/qproperty/tst_qproperty.cpp  | 17 +++++++++++++++++
 3 files changed, 23 insertions(+)

diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp
index ea7635a58f2..1080079dcfb 100644
--- a/src/corelib/kernel/qproperty.cpp
+++ b/src/corelib/kernel/qproperty.cpp
@@ -566,6 +566,11 @@ void QPropertyBindingData::registerWithCurrentlyEvaluatingBinding_helper(Binding
 {
     QPropertyBindingDataPointer d{this};
 
+    if (currentState->alreadyCaptureProperties.contains(this))
+        return;
+    else
+        currentState->alreadyCaptureProperties.push_back(this);
+
     QPropertyObserverPointer dependencyObserver = currentState->binding->allocateDependencyObserver();
     Q_ASSERT(QPropertyObserver::ObserverNotifiesBinding == 0);
     dependencyObserver.setBindingToNotify_unsafe(currentState->binding);
diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h
index 43bd81061dd..b850fa88326 100644
--- a/src/corelib/kernel/qproperty_p.h
+++ b/src/corelib/kernel/qproperty_p.h
@@ -144,6 +144,7 @@ struct BindingEvaluationState
     QPropertyBindingPrivate *binding;
     BindingEvaluationState *previousState = nullptr;
     BindingEvaluationState **currentState = nullptr;
+    QVarLengthArray<const QPropertyBindingData *, 8> alreadyCaptureProperties;
 };
 
 /*!
diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
index 8d6781fa2d1..63cf9c28174 100644
--- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
+++ b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
@@ -99,6 +99,7 @@ private slots:
     void metaProperty();
 
     void modifyObserverListWhileIterating();
+    void noDoubleCapture();
     void compatPropertyNoDobuleNotification();
     void compatPropertySignals();
 
@@ -1491,6 +1492,22 @@ void tst_QProperty::modifyObserverListWhileIterating()
     }
 }
 
+void tst_QProperty::noDoubleCapture()
+{
+    QProperty<long long> size;
+    size = 3;
+    QProperty<int> max;
+    max.setBinding([&size]() -> int {
+        // each loop run attempts to capture size
+        for (int i = 0; i < size; ++i) {}
+        return size.value();
+    });
+    auto bindingPriv = QPropertyBindingPrivate::get(max.binding());
+    QCOMPARE(bindingPriv->dependencyObserverCount, 1);
+    size = 4; // should not crash
+    QCOMPARE(max.value(), 4);
+}
+
 class CompatPropertyTester : public QObject
 {
     Q_OBJECT