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>
This commit is contained in:
Fabian Kosmale 2023-04-18 08:54:36 +02:00 committed by Qt Cherry-pick Bot
parent 8af1698891
commit b7c3eabdb8
3 changed files with 23 additions and 0 deletions

View File

@ -566,6 +566,11 @@ void QPropertyBindingData::registerWithCurrentlyEvaluatingBinding_helper(Binding
{ {
QPropertyBindingDataPointer d{this}; QPropertyBindingDataPointer d{this};
if (currentState->alreadyCaptureProperties.contains(this))
return;
else
currentState->alreadyCaptureProperties.push_back(this);
QPropertyObserverPointer dependencyObserver = currentState->binding->allocateDependencyObserver(); QPropertyObserverPointer dependencyObserver = currentState->binding->allocateDependencyObserver();
Q_ASSERT(QPropertyObserver::ObserverNotifiesBinding == 0); Q_ASSERT(QPropertyObserver::ObserverNotifiesBinding == 0);
dependencyObserver.setBindingToNotify_unsafe(currentState->binding); dependencyObserver.setBindingToNotify_unsafe(currentState->binding);

View File

@ -144,6 +144,7 @@ struct BindingEvaluationState
QPropertyBindingPrivate *binding; QPropertyBindingPrivate *binding;
BindingEvaluationState *previousState = nullptr; BindingEvaluationState *previousState = nullptr;
BindingEvaluationState **currentState = nullptr; BindingEvaluationState **currentState = nullptr;
QVarLengthArray<const QPropertyBindingData *, 8> alreadyCaptureProperties;
}; };
/*! /*!

View File

@ -99,6 +99,7 @@ private slots:
void metaProperty(); void metaProperty();
void modifyObserverListWhileIterating(); void modifyObserverListWhileIterating();
void noDoubleCapture();
void compatPropertyNoDobuleNotification(); void compatPropertyNoDobuleNotification();
void compatPropertySignals(); 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 class CompatPropertyTester : public QObject
{ {
Q_OBJECT Q_OBJECT