Avoid accessing deleted binding data in grouped updates

This fixes a use-after-free in QPropertyDelayedNotifications::notify.

Before this patch, evaluateBindings or a notify from a property index
might have caused the originalBindingData to become reallocated.
However, at that point, we've already restored the original bindingData
in evaluateBindings, so we won't track updates, and thus won't adjust
originalBindingStatus, which will then point to already freed data.

To remedy this, we no longer do the notification with data fetched from
originalBindingData, but instead use the information we have in the
proxyData.
We also need to enure that referenced bindings do not get deleted; for
that we keep the PendingBindingObserverList alive for the whole duration
of the endPropertyUpdateGroup.

As we now have the PendingBindingObserverList, we use it for the
notification logic, and only notify change handlers in
QPropertyDelayedNotifications::notify. That will allow a follow-up
cleanup of QPropertyObserverPointer::notify, and aligns the logic for
grouped updates with the logic for "nornal", non-grouped updates.

Amends f1b1773d0ae636fa9afa36224ba17566484af3cc.

Task-number: QTBUG-110899
Change-Id: Iae826e620d9614b7df39d86d8a28c48c8a5c4881
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
(cherry picked from commit 7a415a051a464ee3145c11b4ff44dbb16010323e)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

2 files changed, 44 insertions, 16 deletions

diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp
index b8b5c0bdc7..9ae78669b9 100644
--- a/src/corelib/kernel/qproperty.cpp
+++ b/src/corelib/kernel/qproperty.cpp
@@ -118,12 +118,11 @@ struct QPropertyDelayedNotifications
         Change notifications are sent later with notify (following the logic of separating
         binding updates and notifications used in non-deferred updates).
      */
-     [[nodiscard]] PendingBindingObserverList evaluateBindings(qsizetype index, QBindingStatus *status) {
-        PendingBindingObserverList bindingObservers;
+     void evaluateBindings(PendingBindingObserverList &bindingObservers, qsizetype index, QBindingStatus *status) {
         auto *delayed = delayedProperties + index;
         auto *bindingData = delayed->originalBindingData;
         if (!bindingData)
-            return bindingObservers;
+            return;
 
         bindingData->d_ptr = delayed->d_ptr;
         Q_ASSERT(!(bindingData->d_ptr & QPropertyBindingData::DelayedNotificationBit));
@@ -136,7 +135,6 @@ struct QPropertyDelayedNotifications
         QPropertyObserverPointer observer = bindingDataPointer.firstObserver();
         if (observer)
             observer.evaluateBindings(bindingObservers, status);
-        return bindingObservers;
     }
 
     /*!
@@ -150,17 +148,17 @@ struct QPropertyDelayedNotifications
      */
     void notify(qsizetype index) {
         auto *delayed = delayedProperties + index;
-        auto *bindingData = delayed->originalBindingData;
-        if (!bindingData)
+        if (delayed->d_ptr  & QPropertyBindingData::BindingBit)
+            return; // already handled
+        if (!delayed->originalBindingData)
             return;
-
         delayed->originalBindingData = nullptr;
+
+        QPropertyObserverPointer observer  { reinterpret_cast<QPropertyObserver *>(delayed->d_ptr & ~QPropertyBindingData::DelayedNotificationBit) };
         delayed->d_ptr = 0;
 
-        QPropertyBindingDataPointer bindingDataPointer{bindingData};
-        QPropertyObserverPointer observer = bindingDataPointer.firstObserver();
         if (observer)
-            observer.notify(delayed->propertyData);
+            observer.notify<QPropertyObserverPointer::Notify::OnlyChangeHandlers>(delayed->propertyData);
     }
 };
 
@@ -215,17 +213,21 @@ void Qt::endPropertyUpdateGroup()
     if (--data->ref)
         return;
     groupUpdateData = nullptr;
+    // ensures that bindings are kept alive until endPropertyUpdateGroup concludes
+    PendingBindingObserverList bindingObservers;
     // update all delayed properties
     auto start = data;
     while (data) {
-        for (qsizetype i = 0; i < data->used; ++i) {
-            PendingBindingObserverList bindingObserves = data->evaluateBindings(i, status);
-            Q_UNUSED(bindingObserves);
-            // ### TODO: Use bindingObservers for notify
-        }
+        for (qsizetype i = 0; i < data->used; ++i)
+            data->evaluateBindings(bindingObservers, i, status);
         data = data->next;
     }
-    // notify all delayed properties
+    // notify all delayed notifications from binding evaluation
+    for (const QBindingObserverPtr &observer: bindingObservers) {
+        QPropertyBindingPrivate *binding = observer.binding();
+        binding->notifyNonRecursive();
+    }
+    // do the same for properties which only have observers
     data = start;
     while (data) {
         for (qsizetype i = 0; i < data->used; ++i)
diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
index 97dda5f437..18bdcb9dc7 100644
--- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
+++ b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
@@ -87,6 +87,7 @@ private slots:
     void groupedNotifications();
     void groupedNotificationConsistency();
     void bindingGroupMovingBindingData();
+    void bindingGroupBindingDeleted();
     void uninstalledBindingDoesNotEvaluate();
 
     void notify();
@@ -1976,6 +1977,31 @@ void tst_QProperty::bindingGroupMovingBindingData()
     QVERIFY(proxyData);
 }
 
+void tst_QProperty::bindingGroupBindingDeleted()
+{
+    auto deleter = std::make_unique<ClassWithNotifiedProperty>();
+    auto toBeDeleted = std::make_unique<ClassWithNotifiedProperty>();
+
+    bool calledHandler = false;
+    deleter->property.setBinding([&](){
+        int newValue = toBeDeleted->property;
+        if (newValue == 42)
+            toBeDeleted.reset();
+        return newValue;
+    });
+    auto handler = toBeDeleted->property.onValueChanged([&]() { calledHandler = true; } );
+    {
+        Qt::beginPropertyUpdateGroup();
+        auto cleanup = qScopeGuard([](){ Qt::endPropertyUpdateGroup(); });
+        QVERIFY(toBeDeleted);
+        toBeDeleted->property = 42;
+        // ASAN should not complain here
+    }
+    QVERIFY(!toBeDeleted);
+    // the change notification is sent, even if the binding is deleted during evaluation
+    QVERIFY(calledHandler);
+}
+
 void tst_QProperty::uninstalledBindingDoesNotEvaluate()
 {
     QProperty<int> i;