From be974c485559ed1c085a9b43b8c91649d0e08656 Mon Sep 17 00:00:00 2001 From: Santhosh Kumar Date: Wed, 25 Sep 2024 17:18:40 +0200 Subject: [PATCH] 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 Pick-to: 6.5 6.2 Change-Id: I8c97721ca563083d6280194175f8a915dac2ff04 Reviewed-by: Ulf Hermann Reviewed-by: Santhosh Kumar (cherry picked from commit 275202215376238bf414d5008d3c727085e58a4c) Reviewed-by: Qt Cherry-pick Bot --- src/corelib/kernel/qproperty.cpp | 23 +++++++++++-------- src/corelib/kernel/qproperty_p.h | 8 ++++--- src/corelib/kernel/qpropertyprivate.h | 4 ++-- .../kernel/qproperty/tst_qproperty.cpp | 23 +++++++++++++++++++ 4 files changed, 44 insertions(+), 14 deletions(-) diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp index caa9fce787b..b15bc0a2f75 100644 --- a/src/corelib/kernel/qproperty.cpp +++ b/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(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(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(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(); } diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h index f4c8e69c889..2ecba93302e 100644 --- a/src/corelib/kernel/qproperty_p.h +++ b/src/corelib/kernel/qproperty_p.h @@ -59,7 +59,7 @@ public: inline QPropertyObserver *operator ->(); }; -using PendingBindingObserverList = QVarLengthArray; +using PendingBindingObserverList = QVarLengthArray; // 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 @@ public: // 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(bindingPtr.get()); + binding->notifyNonRecursive(); + } } } } diff --git a/src/corelib/kernel/qpropertyprivate.h b/src/corelib/kernel/qpropertyprivate.h index c4a73f2c912..785a1793152 100644 --- a/src/corelib/kernel/qpropertyprivate.h +++ b/src/corelib/kernel/qpropertyprivate.h @@ -30,8 +30,8 @@ class QBindingStorage; template class QObjectCompatProperty; -struct QBindingObserverPtr; -using PendingBindingObserverList = QVarLengthArray; +class QPropertyBindingPrivatePtr; +using PendingBindingObserverList = QVarLengthArray; namespace QtPrivate { // QPropertyBindingPrivatePtr operates on a RefCountingMixin solely so that we can inline diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp index cc7edb8bf2d..c85ac17ce9b 100644 --- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp +++ b/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 dynamicProperty1(false); + QProperty dynamicProperty2(false); + QProperty dynamicProperty3(false); + QProperty dynamicProperty4(false); + QProperty dynamicProperty5(false); + QProperty dynamicProperty6(false); + QProperty 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