From 652062dde33ddb0ecaf4bb9f38055d8ab87c462b Mon Sep 17 00:00:00 2001
From: Fabian Kosmale <fabian.kosmale@qt.io>
Date: Tue, 21 Jul 2020 11:34:44 +0200
Subject: QNotifiedProperty: avoid crash

We can end up in a situation where a (soon to be destroyed) observer is
owned by a binding which is about to be deleted. If in that situation
the binding is destroyed first, we end up with a dangling pointer
and ensuing memory corruption. Instead, we now first transfer the
ownership of the observer and only destroy the binding afterwards.

Fixes: QTBUG-85824
Change-Id: I721c0319281ada981ae7896bd2e02e9a0cc901b8
Reviewed-by: Lars Knoll <lars.knoll@qt.io>
---
 src/corelib/kernel/qproperty.cpp                   |  2 +-
 .../corelib/kernel/qproperty/tst_qproperty.cpp     | 29 ++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp
index 3bac73909a..cbb2954a98 100644
--- a/src/corelib/kernel/qproperty.cpp
+++ b/src/corelib/kernel/qproperty.cpp
@@ -338,10 +338,10 @@ void QPropertyBase::removeBinding()
 
     if (auto *existingBinding = d.bindingPtr()) {
         auto observer = existingBinding->takeObservers();
-        existingBinding->unlinkAndDeref();
         d_ptr &= ExtraBit;
         if (observer)
             d.setObservers(observer.ptr);
+        existingBinding->unlinkAndDeref();
     }
 }
 
diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
index 41cf60b55f..74a81cc93b 100644
--- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
+++ b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
@@ -73,6 +73,7 @@ private slots:
     void notifiedPropertyWithOldValueCallback();
     void notifiedPropertyWithGuard();
     void typeNoOperatorEqual();
+    void bindingValueReplacement();
 };
 
 void tst_QProperty::functorBinding()
@@ -1058,6 +1059,34 @@ void tst_QProperty::typeNoOperatorEqual()
     QVERIFY(u1.changedCalled);
 }
 
+
+struct Test {
+    void notify() {};
+    bool bindText(int);
+    bool bindIconText(int);
+    QProperty<int> text;
+    QNotifiedProperty<int, &Test::notify, &Test::bindIconText> iconText;
+};
+
+bool Test::bindIconText(int) {
+    Q_UNUSED(iconText.value()); // force read
+    if (!iconText.hasBinding()) {
+        iconText.setBinding(this, [=]() { return 0; });
+    }
+    return true;
+}
+
+void tst_QProperty::bindingValueReplacement()
+{
+    Test test;
+    test.text = 0;
+    test.bindIconText(0);
+    test.iconText.setValue(&test, 42); // should not crash
+    QCOMPARE(test.iconText.value(), 42);
+    test.text = 1;
+    QCOMPARE(test.iconText.value(), 42);
+}
+
 QTEST_MAIN(tst_QProperty);
 
 #include "tst_qproperty.moc"
-- 
cgit v1.2.3