Handle notifier list modification during iteration

As propertyobservers can execute arbitrarily complex code, they can also
modify the obsever list in multiple ways. To protect against list
corruption resulting from this, we introduce a protection scheme which
makes the list resilient against modification.
A detailed description of the scheme can be found as a comment in
QPropertyObserverPointer::notify.

Task-number: QTBUG-87153
Change-Id: I9bb49e457165ddc1e4c8bbdf3d3c9fbf5ff27e94
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Lars Knoll <lars.knoll@qt.io>

src/corelib/kernel/qproperty.cpp

@@ -370,22 +370,27 @@ void QPropertyObserver::setSource(const QPropertyBindingData &property)
     d.observeProperty(propPrivate);
 }
 
-
 QPropertyObserver::~QPropertyObserver()
 {
+    if (next.tag() == ActivelyExecuting) {
+        if (nodeState)
+            *nodeState = nullptr;
+    }
     QPropertyObserverPointer d{this};
     d.unlink();
 }
 
 QPropertyObserver::QPropertyObserver(QPropertyObserver &&other)
 {
-    std::swap(bindingToMarkDirty, other.bindingToMarkDirty);
-    std::swap(next, other.next);
-    std::swap(prev, other.prev);
+    bindingToMarkDirty = std::exchange(other.bindingToMarkDirty, {});
+    next = std::exchange(other.next, {});
+    prev = std::exchange(other.prev, {});
     if (next)
         next->prev = &next;
     if (prev)
         prev.setPointer(this);
+    if (next.tag() == ActivelyExecuting)
+        *nodeState = this;
 }
 
 QPropertyObserver &QPropertyObserver::operator=(QPropertyObserver &&other)
@@ -397,20 +402,22 @@ QPropertyObserver &QPropertyObserver::operator=(QPropertyObserver &&other)
     d.unlink();
     bindingToMarkDirty = nullptr;
 
-    std::swap(bindingToMarkDirty, other.bindingToMarkDirty);
-    std::swap(next, other.next);
-    std::swap(prev, other.prev);
+    bindingToMarkDirty = std::exchange(other.bindingToMarkDirty, {});
+    next = std::exchange(other.next, {});
+    prev = std::exchange(other.prev, {});
     if (next)
         next->prev = &next;
     if (prev)
         prev.setPointer(this);
+    if (next.tag() == ActivelyExecuting)
+        *nodeState = this;
 
     return *this;
 }
 
 void QPropertyObserverPointer::unlink()
 {
-    if (ptr->next.tag() & QPropertyObserver::ObserverNotifiesAlias)
+    if (ptr->next.tag() == QPropertyObserver::ObserverNotifiesAlias)
         ptr->aliasedPropertyData = nullptr;
     if (ptr->next)
         ptr->next->prev = ptr->prev;
@@ -422,22 +429,81 @@ void QPropertyObserverPointer::unlink()
 
 void QPropertyObserverPointer::setChangeHandler(QPropertyObserver::ChangeHandler changeHandler)
 {
+    Q_ASSERT(ptr->next.tag() != QPropertyObserver::ActivelyExecuting);
     ptr->changeHandler = changeHandler;
     ptr->next.setTag(QPropertyObserver::ObserverNotifiesChangeHandler);
 }
 
 void QPropertyObserverPointer::setAliasedProperty(QUntypedPropertyData *property)
 {
+    Q_ASSERT(ptr->next.tag() != QPropertyObserver::ActivelyExecuting);
     ptr->aliasedPropertyData = property;
     ptr->next.setTag(QPropertyObserver::ObserverNotifiesAlias);
 }
 
 void QPropertyObserverPointer::setBindingToMarkDirty(QPropertyBindingPrivate *binding)
 {
+    Q_ASSERT(ptr->next.tag() != QPropertyObserver::ActivelyExecuting);
     ptr->bindingToMarkDirty = binding;
     ptr->next.setTag(QPropertyObserver::ObserverNotifiesBinding);
 }
 
+/*!
+ \internal
+ QPropertyObserverNodeProtector is a RAII wrapper which takes care of the internal switching logic
+ for QPropertyObserverPointer::notify (described ibidem)
+*/
+template <QPropertyObserver::ObserverTag tag>
+struct QPropertyObserverNodeProtector {
+    QPropertyObserver m_placeHolder;
+    QPropertyObserver *&m_observer;
+    union {
+        QPropertyBindingPrivate *m_binding;
+        QPropertyObserver::ChangeHandler m_changeHandler;
+    };
+    Q_REQUIRED_RESULT QPropertyObserverNodeProtector(QPropertyObserver *&observer)
+        : m_observer(observer)
+    {
+        static_assert(tag == QPropertyObserver::ObserverNotifiesBinding ||
+                 tag == QPropertyObserver::ObserverNotifiesChangeHandler);
+        if constexpr (tag == QPropertyObserver::ObserverNotifiesBinding)
+            m_binding = m_observer->bindingToMarkDirty;
+        else
+            m_changeHandler = m_observer->changeHandler;
+        switchNodes(m_placeHolder, m_observer);
+        m_observer->nodeState = &m_observer;
+        m_observer->next.setTag(QPropertyObserver::ActivelyExecuting);
+        m_placeHolder.next.setTag(QPropertyObserver::ActivelyExecuting);
+    }
+
+    ~QPropertyObserverNodeProtector() {
+        if (m_observer) {
+            if constexpr (tag == QPropertyObserver::ObserverNotifiesBinding)
+                m_observer->bindingToMarkDirty = m_binding;
+            else
+                m_observer->changeHandler = m_changeHandler;
+            switchNodes(*m_observer, &m_placeHolder);
+            m_observer->next.setTag(tag);
+        }
+        // set tag to a safer value where we don't execute anything in the dtor
+        m_placeHolder.next.setTag(QPropertyObserver::ObserverNotifiesChangeHandler);
+    }
+
+    /*!
+      \internal
+      replaces a node \a observer in the list with another node \a placeholder which must not be in the list
+     */
+    static void switchNodes(QPropertyObserver &placeHolder, QPropertyObserver *observer)  {
+        placeHolder.next = std::exchange(observer->next, {});
+        placeHolder.prev = std::exchange(observer->prev, {});
+        if (placeHolder.next) {
+            placeHolder.next->prev = &placeHolder.next;
+        }
+        if (placeHolder.prev)
+            placeHolder.prev.setPointer(&placeHolder);
+    };
+};
+
 /*! \internal
   \a propertyDataPtr is a pointer to the observed property's property data
   In case that property has a binding, \a triggeringBinding points to the binding's QPropertyBindingPrivate
@@ -453,29 +519,62 @@ void QPropertyObserverPointer::notify(QPropertyBindingPrivate *triggeringBinding
     bool propertyChanged = true;
 
     auto observer = const_cast<QPropertyObserver*>(ptr);
+    /*
+     * The basic idea of the loop is as follows: We iterate over all observers in the linked list,
+     * and execute the functionality corresponding to their tag.
+     * However, complication arise due to the fact that the triggered operations might modify the list,
+     * which includes deletion and move of the current and next nodes.
+     * Therefore, we take a few safety precautions:
+     * 1. Before executing any action which might modify the list, we replace the actual node in the list with
+     *    a placeholder node. As that one is stack allocated and owned by us, we can rest assured that it is
+     *    still there after the action has executed, and placeHolder->next points to the actual next node in the list.
+     *    Note that taking next at the beginning of the loop does not work, as the execuated action might either move
+     *    or delete that node.
+     * 2. To properly handle deletion or moves of the real current node, we store a pointer to a pointer to itself in
+     *    its nodeState. Whenever the node is reallocated and moved, we update that pointer to point to its new
+     *    location. If the node is actually deleted, we set it to nullptr.
+     * 3. After the triggered action has finished, we can use that information to restore the list to contain the actual
+     *    node again. We either switch the nodes with the real nodes current location, or, if the real node has been
+     *    deleted, we simply unlink the temporary node.
+     */
     while (observer) {
-        auto * const next = observer->next.data();
+        QPropertyObserver *next = nullptr;
+        char preventBug[1] = {'\0'}; // QTBUG-87245
+        Q_UNUSED(preventBug);
         switch (observer->next.tag()) {
         case QPropertyObserver::ObserverNotifiesChangeHandler:
-            if (!knownIfPropertyChanged && triggeringBinding) {
-                knownIfPropertyChanged = true;
-
-                propertyChanged = triggeringBinding->evaluateIfDirtyAndReturnTrueIfValueChanged(propertyDataPtr);
-            }
-            if (!propertyChanged)
-                return;
-
-            if (auto handlerToCall = std::exchange(observer->changeHandler, nullptr)) {
+            if (auto handlerToCall = observer->changeHandler) {
+                // both evaluateIfDirtyAndReturnTrueIfValueChanged and handlerToCall might modify the list
+                QPropertyObserverNodeProtector<QPropertyObserver::ObserverNotifiesChangeHandler> protector(observer);
+                if (!knownIfPropertyChanged && triggeringBinding) {
+                    knownIfPropertyChanged = true;
+                    propertyChanged = triggeringBinding->evaluateIfDirtyAndReturnTrueIfValueChanged(propertyDataPtr);
+                }
+                if (!propertyChanged)
+                    return;
                 handlerToCall(observer, propertyDataPtr);
-                observer->changeHandler = handlerToCall;
+                next = protector.m_placeHolder.next.data();
+            } else {
+                next = observer->next.data();
             }
             break;
         case QPropertyObserver::ObserverNotifiesBinding:
-            if (observer->bindingToMarkDirty)
-                observer->bindingToMarkDirty->markDirtyAndNotifyObservers();
+            if (auto bindingToMarkDirty =  observer->bindingToMarkDirty) {
+                QPropertyObserverNodeProtector<QPropertyObserver::ObserverNotifiesBinding> protector(observer);
+                bindingToMarkDirty->markDirtyAndNotifyObservers();
+                next = protector.m_placeHolder.next.data();
+            } else {
+                next = observer->next.data();
+            }
             break;
         case QPropertyObserver::ObserverNotifiesAlias:
+            next = observer->next.data();
             break;
+        case QPropertyObserver::ActivelyExecuting:
+            // recursion is already properly handled somewhere else
+            return;
+        default:
+            Q_UNREACHABLE();
         }
         observer = next;
     }
@@ -1484,5 +1583,4 @@ QPropertyBindingData *QBindingStorage::bindingData(QUntypedPropertyData *data, b
     return storage;
 }
 
-
 QT_END_NAMESPACE

src/corelib/kernel/qproperty.h

@@ -221,9 +221,11 @@ class Q_CORE_EXPORT QPropertyObserver
 public:
     // Internal
     enum ObserverTag {
-        ObserverNotifiesBinding,
-        ObserverNotifiesChangeHandler,
-        ObserverNotifiesAlias,
+        ObserverNotifiesBinding, // observer was installed to notify bindings that obsverved property changed
+        ObserverNotifiesChangeHandler, // observer is a change handler, which runs on every change
+        ObserverNotifiesAlias, // used for QPropertyAlias
+        ActivelyExecuting  // the observer is currently evaluated in QPropertyObserver::notifyObservers or its
+                           // placeholder. We only can store 4 different values, therefore those two conflate
     };
 
     QPropertyObserver() = default;
@@ -257,6 +259,7 @@ private:
         QPropertyBindingPrivate *bindingToMarkDirty = nullptr;
         ChangeHandler changeHandler;
         QUntypedPropertyData *aliasedPropertyData;
+        QPropertyObserver **nodeState;
     };
 
     QPropertyObserver(const QPropertyObserver &) = delete;
@@ -265,6 +268,8 @@ private:
     friend struct QPropertyObserverPointer;
     friend struct QPropertyBindingDataPointer;
     friend class QPropertyBindingPrivate;
+    template<ObserverTag>
+    friend struct QPropertyObserverNodeProtector;
 };
 
 template <typename Functor>

src/corelib/kernel/qproperty_p.h

@@ -241,6 +241,13 @@ public:
     void clearDependencyObservers() {
         for (size_t i = 0; i < qMin(dependencyObserverCount, inlineDependencyObservers.size()); ++i) {
             QPropertyObserverPointer p{&inlineDependencyObservers[i]};
+            if (p.ptr->next.tag() == QPropertyObserver::ActivelyExecuting) {
+                *(p.ptr->nodeState) = nullptr;
+                p.ptr->nodeState = nullptr;
+
+                // set tag to "safer" value, as we return the same observer pointer from allocateDependencyObserver
+                p.ptr->next.setTag(QPropertyObserver::ObserverNotifiesChangeHandler);
+            }
             p.unlink();
         }
         if (heapObservers)

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

@@ -76,6 +76,8 @@ private slots:
     void compatBindings();
     void metaProperty();
     void aliasOnMetaProperty();
+
+    void modifyObserverListWhileIterating();
 };
 
 void tst_QProperty::functorBinding()
@@ -1248,6 +1250,57 @@ void tst_QProperty::aliasOnMetaProperty()
     QCOMPARE(alias.value(), 100);
 }
 
+void tst_QProperty::modifyObserverListWhileIterating()
+{
+        struct DestructingObserver : QPropertyObserver {
+            DestructingObserver() :  QPropertyObserver([](QPropertyObserver *self, QUntypedPropertyData *) {
+                auto This = static_cast<DestructingObserver *>(self);
+                (*This)();
+            }), m_target(this){}
+            void operator()() {
+                (*counter)++;
+                std::destroy_at(m_target);
+            }
+            DestructingObserver *m_target;
+            int *counter = nullptr;
+        };
+        union ObserverOrUninit {
+            DestructingObserver observer = {};
+            char* memory;
+            ~ObserverOrUninit() {}
+        };
+    {
+        // observer deletes itself while running the notification
+        // while explicitly calling the destructor is rather unusual
+        // it is completely plausible for this to happen because the object to which a
+        // propertyobserver belongs has been destroyed
+        ObserverOrUninit data;
+        int counter = 0;
+        data.observer.counter = &counter;
+        QProperty<int> prop;
+        QUntypedBindable bindableProp(&prop);
+        bindableProp.observe(&data.observer);
+        prop = 42; // should not crash
+        QCOMPARE(counter, 1);
+    }
+    {
+        // observer deletes the next observer in the list
+        ObserverOrUninit data1;
+        ObserverOrUninit data2;
+        QProperty<int> prop;
+        QUntypedBindable bindableProp(&prop);
+        bindableProp.observe(&data1.observer);
+        bindableProp.observe(&data2.observer);
+        int counter = 0;
+        data1.observer.m_target = &data2.observer;
+        data1.observer.counter = &counter;
+        data2.observer.m_target = &data1.observer;
+        data2.observer.counter = &counter;
+        prop = 42; // should not crash
+        QCOMPARE(counter, 1); // only one trigger should run as the other has been deleted
+    }
+}
+
 QTEST_MAIN(tst_QProperty);
 
 #include "tst_qproperty.moc"