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>

3 files changed, 135 insertions, 25 deletions

diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp
index 14ee881455..d25952c0bf 100644
--- a/src/corelib/kernel/qproperty.cpp
+++ b/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
diff --git a/src/corelib/kernel/qproperty.h b/src/corelib/kernel/qproperty.h
index 88745d6cde..0d9a34dcde 100644
--- a/src/corelib/kernel/qproperty.h
+++ b/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>
diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h
index aa81aa9203..9185c939c6 100644
--- a/src/corelib/kernel/qproperty_p.h
+++ b/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)