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 f1b1773d0a.

Task-number: QTBUG-110899
Pick-to: 6.5 6.4 6.2
Change-Id: Iae826e620d9614b7df39d86d8a28c48c8a5c4881
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>

src/corelib/kernel/qproperty.cpp

@@ -121,12 +121,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));
@@ -139,7 +138,6 @@ struct QPropertyDelayedNotifications
         QPropertyObserverPointer observer = bindingDataPointer.firstObserver();
         if (observer)
             observer.evaluateBindings(bindingObservers, status);
-        return bindingObservers;
     }
 
     /*!
@@ -153,17 +151,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);
     }
 };
 
@@ -218,17 +216,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)

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

@@ -89,6 +89,7 @@ private slots:
     void groupedNotifications();
     void groupedNotificationConsistency();
     void bindingGroupMovingBindingData();
+    void bindingGroupBindingDeleted();
     void uninstalledBindingDoesNotEvaluate();
 
     void notify();
@@ -2166,6 +2167,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;