From e29ffa179e9920443a23e2fcb3f0694df32e8a68 Mon Sep 17 00:00:00 2001
From: Erik Verbruggen <erik.verbruggen@qt.io>
Date: Thu, 14 Sep 2017 13:47:14 +0200
Subject: Fix use-after-free when removing elements from a ListModel

Detaching delegate instances from model items is done after the
destruction of said model items. The problem is that after the model
item is destroyed, it will emit a change/destroyed signal. As the
delegate is still referencing the item, this will result in a
use-after-free. To provent that, the items are kept around until after
everyone (notably the delegate model) has been notified of the removal.

[ChangeLog][Qt][Qml] Fix possible use-after-free when removing items from a ListModel through JavaScript.

Task-number: QTBUG-59256
Change-Id: Iee182e2cf0b50d3dda2181fed95e38f1a60f22a9
Reviewed-by: Simon Hausmann <simon.hausmann@qt.io>
---
 src/qml/types/qqmldelegatemodel.cpp |  7 +++++++
 src/qml/types/qqmllistmodel.cpp     | 25 +++++++++++++++++++------
 src/qml/types/qqmllistmodel_p_p.h   |  2 +-
 3 files changed, 27 insertions(+), 7 deletions(-)

(limited to 'src')

diff --git a/src/qml/types/qqmldelegatemodel.cpp b/src/qml/types/qqmldelegatemodel.cpp
index 9b1417eccd..26e6a81418 100644
--- a/src/qml/types/qqmldelegatemodel.cpp
+++ b/src/qml/types/qqmldelegatemodel.cpp
@@ -1266,6 +1266,13 @@ void QQmlDelegateModel::_q_itemsInserted(int index, int count)
     d->emitChanges();
 }
 
+//### This method should be split in two. It will remove delegates, and it will re-render the list.
+// When e.g. QQmlListModel::remove is called, the removal of the delegates should be done on
+// QAbstractItemModel::rowsAboutToBeRemoved, and the re-rendering on
+// QAbstractItemModel::rowsRemoved. Currently both are done on the latter signal. The problem is
+// that the destruction of an item will emit a changed signal that ends up at the delegate, which
+// in turn will try to load the data from the model (which should have already freed it), resulting
+// in a use-after-free. See QTBUG-59256.
 void QQmlDelegateModelPrivate::itemsRemoved(
         const QVector<Compositor::Remove> &removes,
         QVarLengthArray<QVector<QQmlChangeSet::Change>, Compositor::MaximumGroupCount> *translatedRemoves,
diff --git a/src/qml/types/qqmllistmodel.cpp b/src/qml/types/qqmllistmodel.cpp
index 9d0f1afb32..b33be2b4fb 100644
--- a/src/qml/types/qqmllistmodel.cpp
+++ b/src/qml/types/qqmllistmodel.cpp
@@ -567,14 +567,20 @@ void ListModel::clear()
     elements.clear();
 }
 
-void ListModel::remove(int index, int count)
+QVector<std::function<void()>> ListModel::remove(int index, int count)
 {
+    QVector<std::function<void()>> toDestroy;
+    auto layout = m_layout;
     for (int i=0 ; i < count ; ++i) {
-        elements[index+i]->destroy(m_layout);
-        delete elements[index+i];
+        auto element = elements[index+i];
+        toDestroy.append([element, layout](){
+            element->destroy(layout);
+            delete element;
+        });
     }
     elements.remove(index, count);
     updateCacheIndices();
+    return toDestroy;
 }
 
 void ListModel::insert(int elementIndex, QV4::Object *object)
@@ -2053,15 +2059,22 @@ void QQmlListModel::remove(QQmlV4Function *args)
 
         emitItemsAboutToBeRemoved(index, removeCount);
 
+        QVector<std::function<void()>> toDestroy;
         if (m_dynamicRoles) {
-            for (int i=0 ; i < removeCount ; ++i)
-                delete m_modelObjects[index+i];
+            for (int i=0 ; i < removeCount ; ++i) {
+                auto modelObject = m_modelObjects[index+i];
+                toDestroy.append([modelObject](){
+                    delete modelObject;
+                });
+            }
             m_modelObjects.remove(index, removeCount);
         } else {
-            m_listModel->remove(index, removeCount);
+            toDestroy = m_listModel->remove(index, removeCount);
         }
 
         emitItemsRemoved(index, removeCount);
+        for (const auto &destroyer : toDestroy)
+            destroyer();
     } else {
         qmlWarning(this) << tr("remove: incorrect number of arguments");
     }
diff --git a/src/qml/types/qqmllistmodel_p_p.h b/src/qml/types/qqmllistmodel_p_p.h
index cdce78e542..4928ad3725 100644
--- a/src/qml/types/qqmllistmodel_p_p.h
+++ b/src/qml/types/qqmllistmodel_p_p.h
@@ -367,7 +367,7 @@ public:
     void insert(int elementIndex, QV4::Object *object);
 
     void clear();
-    void remove(int index, int count);
+    Q_REQUIRED_RESULT QVector<std::function<void()>> remove(int index, int count);
 
     int appendElement();
     void insertElement(int index);
-- 
cgit v1.2.3