QQuickItemView: fix releaseItem() loops

Calling releaseItem() destroys the item, which emits childrenChanged
for the contentItem, and if at that point anything calls setFooMargin(),
setContentHeight(), returnToBounds(), or many other methods that
indirectly access the visibleItems list, it leads to a crash due to
read after free. Add a releaseVisibleItems() helper method that makes
a copy, clears the original list first, and then releases the items.

Task-number: QTBUG-48394
Task-number: QTBUG-61294
Change-Id: I29e4d3870d33549e8bf789de84c67ab1826fca7d
Reviewed-by: Robin Burchell <robin.burchell@crimson.no>

8 files changed, 63 insertions, 12 deletions

diff --git a/src/quick/items/qquickgridview.cpp b/src/quick/items/qquickgridview.cpp
index fd78c46a16..c570b95a21 100644
--- a/src/quick/items/qquickgridview.cpp
+++ b/src/quick/items/qquickgridview.cpp
@@ -495,9 +495,7 @@ bool QQuickGridViewPrivate::addVisibleItems(qreal fillFrom, qreal fillTo, qreal
         // We've jumped more than a page.  Estimate which items are now
         // visible and fill from there.
         int count = (fillFrom - (rowPos + rowSize())) / (rowSize()) * columns;
-        for (FxViewItem *item : qAsConst(visibleItems))
-            releaseItem(item);
-        visibleItems.clear();
+        releaseVisibleItems();
         modelIndex += count;
         if (modelIndex >= model->count())
             modelIndex = model->count() - 1;
diff --git a/src/quick/items/qquickitemview.cpp b/src/quick/items/qquickitemview.cpp
index 555db03962..084b1f197a 100644
--- a/src/quick/items/qquickitemview.cpp
+++ b/src/quick/items/qquickitemview.cpp
@@ -380,9 +380,7 @@ void QQuickItemView::setDelegate(QQmlComponent *delegate)
         int oldCount = dataModel->count();
         dataModel->setDelegate(delegate);
         if (isComponentComplete()) {
-            for (FxViewItem *item : qAsConst(d->visibleItems))
-                d->releaseItem(item);
-            d->visibleItems.clear();
+            d->releaseVisibleItems();
             d->releaseItem(d->currentItem);
             d->currentItem = 0;
             d->updateSectionCriteria();
@@ -1743,9 +1741,7 @@ void QQuickItemViewPrivate::clear()
     currentChanges.reset();
     timeline.clear();
 
-    for (FxViewItem *item : qAsConst(visibleItems))
-        releaseItem(item);
-    visibleItems.clear();
+    releaseVisibleItems();
     visibleIndex = 0;
 
     for (FxViewItem *item : qAsConst(releasePendingTransition)) {
diff --git a/src/quick/items/qquickitemview_p_p.h b/src/quick/items/qquickitemview_p_p.h
index 3087682ac7..b6353246e8 100644
--- a/src/quick/items/qquickitemview_p_p.h
+++ b/src/quick/items/qquickitemview_p_p.h
@@ -269,6 +269,15 @@ public:
         q->polish();
     }
 
+    void releaseVisibleItems() {
+        // make a copy and clear the visibleItems first to avoid destroyed
+        // items being accessed during the loop (QTBUG-61294)
+        const QList<FxViewItem *> oldVisible = visibleItems;
+        visibleItems.clear();
+        for (FxViewItem *item : oldVisible)
+            releaseItem(item);
+    }
+
     QPointer<QQmlInstanceModel> model;
     QVariant modelVariant;
     int itemCount;
diff --git a/src/quick/items/qquicklistview.cpp b/src/quick/items/qquicklistview.cpp
index f739115e6b..18f9b8512d 100644
--- a/src/quick/items/qquicklistview.cpp
+++ b/src/quick/items/qquicklistview.cpp
@@ -660,9 +660,7 @@ bool QQuickListViewPrivate::addVisibleItems(qreal fillFrom, qreal fillTo, qreal
         int newModelIdx = qBound(0, modelIndex + count, model->count());
         count = newModelIdx - modelIndex;
         if (count) {
-            for (FxViewItem *item : qAsConst(visibleItems))
-                releaseItem(item);
-            visibleItems.clear();
+            releaseVisibleItems();
             modelIndex = newModelIdx;
             visibleIndex = modelIndex;
             visiblePos = itemEnd + count * (averageSize + spacing);
diff --git a/tests/auto/quick/qquickgridview/data/releaseItems.qml b/tests/auto/quick/qquickgridview/data/releaseItems.qml
new file mode 100644
index 0000000000..19d58550a4
--- /dev/null
+++ b/tests/auto/quick/qquickgridview/data/releaseItems.qml
@@ -0,0 +1,12 @@
+import QtQuick 2.0
+
+GridView {
+    width: 400
+    height: 400
+    model: 100
+    delegate: Rectangle {
+        height: 100; width: 100
+        color: index % 2 ? "lightsteelblue" : "lightgray"
+    }
+    contentHeight: contentItem.children.length * 40
+}
diff --git a/tests/auto/quick/qquickgridview/tst_qquickgridview.cpp b/tests/auto/quick/qquickgridview/tst_qquickgridview.cpp
index 1acc36c9b0..b2d6584701 100644
--- a/tests/auto/quick/qquickgridview/tst_qquickgridview.cpp
+++ b/tests/auto/quick/qquickgridview/tst_qquickgridview.cpp
@@ -209,6 +209,7 @@ private slots:
     void QTBUG_48870_fastModelUpdates();
 
     void keyNavigationEnabled();
+    void releaseItems();
 
 private:
     QList<int> toIntList(const QVariantList &list);
@@ -6666,6 +6667,18 @@ void tst_QQuickGridView::QTBUG_48870_fastModelUpdates()
     }
 }
 
+void tst_QQuickGridView::releaseItems()
+{
+    QScopedPointer<QQuickView> view(createView());
+    view->setSource(testFileUrl("releaseItems.qml"));
+
+    QQuickGridView *gridview = qobject_cast<QQuickGridView *>(view->rootObject());
+    QVERIFY(gridview);
+
+    // don't crash (QTBUG-61294)
+    gridview->setModel(123);
+}
+
 QTEST_MAIN(tst_QQuickGridView)
 
 #include "tst_qquickgridview.moc"
diff --git a/tests/auto/quick/qquicklistview/data/releaseItems.qml b/tests/auto/quick/qquicklistview/data/releaseItems.qml
new file mode 100644
index 0000000000..de774e5e08
--- /dev/null
+++ b/tests/auto/quick/qquicklistview/data/releaseItems.qml
@@ -0,0 +1,12 @@
+import QtQuick 2.0
+
+ListView {
+    width: 400
+    height: 400
+    model: 100
+    delegate: Rectangle {
+        height: 40; width: 400
+        color: index % 2 ? "lightsteelblue" : "lightgray"
+    }
+    contentHeight: contentItem.children.length * 40
+}
diff --git a/tests/auto/quick/qquicklistview/tst_qquicklistview.cpp b/tests/auto/quick/qquicklistview/tst_qquicklistview.cpp
index ff06c1e1a4..98c628068d 100644
--- a/tests/auto/quick/qquicklistview/tst_qquicklistview.cpp
+++ b/tests/auto/quick/qquicklistview/tst_qquicklistview.cpp
@@ -254,6 +254,7 @@ private slots:
     void keyNavigationEnabled();
     void QTBUG_50097_stickyHeader_positionViewAtIndex();
     void itemFiltered();
+    void releaseItems();
 
 private:
     template <class T> void items(const QUrl &source);
@@ -8508,6 +8509,18 @@ void tst_QQuickListView::itemFiltered()
     model.setData(model.index(2), QStringLiteral("modified three"), Qt::DisplayRole);
 }
 
+void tst_QQuickListView::releaseItems()
+{
+    QScopedPointer<QQuickView> view(createView());
+    view->setSource(testFileUrl("releaseItems.qml"));
+
+    QQuickListView *listview = qobject_cast<QQuickListView *>(view->rootObject());
+    QVERIFY(listview);
+
+    // don't crash (QTBUG-61294)
+    listview->setModel(123);
+}
+
 QTEST_MAIN(tst_QQuickListView)
 
 #include "tst_qquicklistview.moc"