Avoid referring to invalidated observer during binding evaluation

The property observer reference used during binding evaluation can
become invalidated as the container used to maintain the
the observer would invalidate the existing observers during its
resize, leading to referring to invalid memory.

Recognize that we do not need to store the observer, but only its
binding - the observer itself is not used later.

Therefore, change the PendingBindingObserverList to become a list of
QPropertyBindingPrivatePtr to avoid the issue.

Fixes: QTBUG-127596
Original-patch-by: Santhosh Kumar <santhosh.kumar.selvaraj@qt.io>
Pick-to: 6.8 6.5 6.2
Change-Id: I8c97721ca563083d6280194175f8a915dac2ff04
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
Reviewed-by: Santhosh Kumar <santhosh.kumar.selvaraj@qt.io>

src/corelib/kernel/qproperty.cpp

@@ -226,8 +226,8 @@ void Qt::endPropertyUpdateGroup()
         data = data->next;
     }
     // notify all delayed notifications from binding evaluation
-    for (const QBindingObserverPtr &observer: bindingObservers) {
-        QPropertyBindingPrivate *binding = observer.binding();
+    for (const auto &bindingPtr: bindingObservers) {
+        auto *binding = static_cast<QPropertyBindingPrivate *>(bindingPtr.get());
         binding->notifyNonRecursive();
     }
     // do the same for properties which only have observers
@@ -324,8 +324,9 @@ bool QPropertyBindingPrivate::evaluateRecursive(PendingBindingObserverList &bind
 void QPropertyBindingPrivate::notifyNonRecursive(const PendingBindingObserverList &bindingObservers)
 {
     notifyNonRecursive();
-    for (auto &&bindingObserver: bindingObservers) {
-        bindingObserver.binding()->notifyNonRecursive();
+    for (auto &&bindingPtr: bindingObservers) {
+        auto *binding = static_cast<QPropertyBindingPrivate *>(bindingPtr.get());
+        binding->notifyNonRecursive();
     }
 }
 
@@ -639,8 +640,10 @@ void QPropertyBindingData::notifyObservers(QUntypedPropertyData *propertyDataPtr
                 d = QPropertyBindingDataPointer {storage->bindingData(propertyDataPtr)};
             if (QPropertyObserverPointer observer = d.firstObserver())
                 observer.notify(propertyDataPtr);
-            for (auto &&bindingObserver: bindingObservers)
-                bindingObserver.binding()->notifyNonRecursive();
+            for (auto &&bindingPtr: bindingObservers) {
+                auto *binding = static_cast<QPropertyBindingPrivate *>(bindingPtr.get());
+                binding->notifyNonRecursive();
+            }
         }
     }
 }
@@ -805,9 +808,11 @@ void QPropertyObserverPointer::evaluateBindings(PendingBindingObserverList &bind
         if (QPropertyObserver::ObserverTag(observer->next.tag()) == QPropertyObserver::ObserverNotifiesBinding) {
             auto bindingToEvaluate = observer->binding;
             QPropertyObserverNodeProtector protector(observer);
-            QBindingObserverPtr bindingObserver(observer); // binding must not be gone after evaluateRecursive_inline
-            if (bindingToEvaluate->evaluateRecursive_inline(bindingObservers, status))
-                bindingObservers.push_back(std::move(bindingObserver));
+            // binding must not be gone after evaluateRecursive_inline
+            QPropertyBindingPrivatePtr currentBinding(observer->binding);
+            const bool evalStatus = bindingToEvaluate->evaluateRecursive_inline(bindingObservers, status);
+            if (evalStatus)
+                bindingObservers.push_back(std::move(currentBinding));
             next = protector.next();
         }
 

src/corelib/kernel/qproperty_p.h

@@ -59,7 +59,7 @@ struct QBindingObserverPtr
     inline QPropertyObserver *operator ->();
 };
 
-using PendingBindingObserverList = QVarLengthArray<QBindingObserverPtr>;
+using PendingBindingObserverList = QVarLengthArray<QPropertyBindingPrivatePtr>;
 
 // Keep all classes related to QProperty in one compilation unit. Performance of this code is crucial and
 // we need to allow the compiler to inline where it makes sense.
@@ -665,8 +665,10 @@ class QObjectCompatProperty : public QPropertyData<T>
                             // evaluateBindings() can trash the observers. We need to re-fetch here.
                             if (QPropertyObserverPointer observer = d.firstObserver())
                                 observer.notify(this);
-                            for (auto&& bindingObserver: bindingObservers)
-                                bindingObserver.binding()->notifyNonRecursive();
+                            for (auto&& bindingPtr: bindingObservers) {
+                                auto *binding = static_cast<QPropertyBindingPrivate *>(bindingPtr.get());
+                                binding->notifyNonRecursive();
+                            }
                         }
                     }
                 }

src/corelib/kernel/qpropertyprivate.h

@@ -30,8 +30,8 @@ class QBindingStorage;
 template<typename Class, typename T, auto Offset, auto Setter, auto Signal, auto Getter>
 class QObjectCompatProperty;
 
-struct QBindingObserverPtr;
-using PendingBindingObserverList = QVarLengthArray<QBindingObserverPtr>;
+class QPropertyBindingPrivatePtr;
+using PendingBindingObserverList = QVarLengthArray<QPropertyBindingPrivatePtr>;
 
 namespace QtPrivate {
 // QPropertyBindingPrivatePtr operates on a RefCountingMixin solely so that we can inline

tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp

@@ -34,6 +34,7 @@ private slots:
     void basicDependencies();
     void multipleDependencies();
     void bindingWithDeletedDependency();
+    void bindingWithInvalidatedPropertyObserver();
     void dependencyChangeDuringDestruction();
     void recursiveDependency();
     void bindingAfterUse();
@@ -252,6 +253,28 @@ void tst_QProperty::bindingWithDeletedDependency()
     QCOMPARE(propertySelector.value(), staticProperty.value());
 }
 
+void tst_QProperty::bindingWithInvalidatedPropertyObserver()
+{
+    QProperty<bool> dynamicProperty1(false);
+    QProperty<bool> dynamicProperty2(false);
+    QProperty<bool> dynamicProperty3(false);
+    QProperty<bool> dynamicProperty4(false);
+    QProperty<bool> dynamicProperty5(false);
+    QProperty<bool> dynamicProperty6(false);
+    QProperty<bool> propertySelector([&]{
+        return (dynamicProperty1.value() && dynamicProperty2.value() && dynamicProperty3.value() &&
+                dynamicProperty4.value() && dynamicProperty5.value() && dynamicProperty6.value());
+    });
+    dynamicProperty1 = true;
+    dynamicProperty2 = true;
+    dynamicProperty3 = true;
+    dynamicProperty4 = true;
+    dynamicProperty5 = true;
+    QCOMPARE(propertySelector.value(), false);
+    dynamicProperty6 = true;
+    QCOMPARE(propertySelector.value(), true);
+}
+
 class ChangeDuringDtorTester : public QObject
 {
     Q_OBJECT