Fix 2 use-after-free problems in the ListModel

This is a combination of 2 commits that went into 5.9. They cannot be
cherry-picked however, because some c++11 constructs were used. Hence
this combo commit, which is a squashed version of those two commits that
have the c++11 bits replaced by Good Old boilerplate code.

Original commit 1 (e29ffa179e9920443a23e2fcb3f0694df32e8a68):

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.

Original commit 2 (163c515783877b8b0ffb8b5c1bab288addee9745):

Fix use-after-free when clear()ing all elements from a ListModel

Same problem as the problem with remove(), so now clear will call into
remove to do the correct thing.

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

Task-number: QTBUG-63383
Change-Id: I9a6bdf65da63b33833f18c80e74ad7bb93409627
Reviewed-by: Simon Hausmann <simon.hausmann@qt.io>

4 files changed, 68 insertions, 39 deletions

diff --git a/src/qml/types/qqmldelegatemodel.cpp b/src/qml/types/qqmldelegatemodel.cpp
index 4a2cd57f55..f7a397688d 100644
--- a/src/qml/types/qqmldelegatemodel.cpp
+++ b/src/qml/types/qqmldelegatemodel.cpp
@@ -1247,6 +1247,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 8ab64b9fe6..40e7be6f56 100644
--- a/src/qml/types/qqmllistmodel.cpp
+++ b/src/qml/types/qqmllistmodel.cpp
@@ -85,6 +85,9 @@ static QString roleTypeName(ListLayout::Role::DataType t)
     return result;
 }
 
+ListModel::ElementDestroyer::~ElementDestroyer()
+{}
+
 const ListLayout::Role &ListLayout::getRoleOrCreate(const QString &key, Role::DataType type)
 {
     QStringHash<Role *>::Node *node = roleHash.findNode(key);
@@ -340,7 +343,9 @@ ListModel::ListModel(ListLayout *layout, QQmlListModel *modelCache, int uid) : m
 
 void ListModel::destroy()
 {
-    clear();
+    Q_FOREACH (ListModel::ElementDestroyer *destroyer, remove(0, elements.count()))
+        delete destroyer;
+
     m_uid = -1;
     m_layout = 0;
     if (m_modelCache && m_modelCache->m_primary == false)
@@ -556,24 +561,28 @@ void ListModel::set(int elementIndex, QV4::Object *object)
     }
 }
 
-void ListModel::clear()
+QVector<ListModel::ElementDestroyer*> ListModel::remove(int index, int count)
 {
-    int elementCount = elements.count();
-    for (int i=0 ; i < elementCount ; ++i) {
-        elements[i]->destroy(m_layout);
-        delete elements[i];
-    }
-    elements.clear();
-}
+    class DestroyerWithLayout: public ElementDestroyer {
+        ListElement *element;
+        ListLayout *layout;
+    public:
+        DestroyerWithLayout(ListElement *element, ListLayout *layout)
+            : element(element)
+            , layout(layout)
+        {}
+        ~DestroyerWithLayout() {
+            element->destroy(layout);
+            delete element;
+        }
+    };
 
-void ListModel::remove(int index, int count)
-{
-    for (int i=0 ; i < count ; ++i) {
-        elements[index+i]->destroy(m_layout);
-        delete elements[index+i];
-    }
+    QVector<ElementDestroyer*> toDestroy;
+    for (int i=0 ; i < count ; ++i)
+        toDestroy.append(new DestroyerWithLayout(elements[index+i], m_layout));
     elements.remove(index, count);
     updateCacheIndices();
+    return toDestroy;
 }
 
 void ListModel::insert(int elementIndex, QV4::Object *object)
@@ -2048,19 +2057,7 @@ int QQmlListModel::count() const
 */
 void QQmlListModel::clear()
 {
-    int cleared = count();
-
-    emitItemsAboutToBeRemoved(0, cleared);
-
-    if (m_dynamicRoles) {
-        for (int i=0 ; i < m_modelObjects.count() ; ++i)
-            delete m_modelObjects[i];
-        m_modelObjects.clear();
-    } else {
-        m_listModel->clear();
-    }
-
-    emitItemsRemoved(0, cleared);
+    removeElements(0, count());
 }
 
 /*!
@@ -2084,20 +2081,40 @@ void QQmlListModel::remove(QQmlV4Function *args)
             return;
         }
 
-        emitItemsAboutToBeRemoved(index, removeCount);
+        removeElements(index, removeCount);
+    } else {
+        qmlInfo(this) << tr("remove: incorrect number of arguments");
+    }
+}
 
-        if (m_dynamicRoles) {
-            for (int i=0 ; i < removeCount ; ++i)
-                delete m_modelObjects[index+i];
-            m_modelObjects.remove(index, removeCount);
-        } else {
-            m_listModel->remove(index, removeCount);
+void QQmlListModel::removeElements(int index, int removeCount)
+{
+    class ModelNodeDestoyer: public ListModel::ElementDestroyer {
+        DynamicRoleModelNode *modelNode;
+    public:
+        ModelNodeDestoyer(DynamicRoleModelNode *modelNode)
+            : modelNode(modelNode)
+        {}
+        ~ModelNodeDestoyer() {
+            delete modelNode;
         }
+    };
+
+    emitItemsAboutToBeRemoved(index, removeCount);
 
-        emitItemsRemoved(index, removeCount);
+    QVector<ListModel::ElementDestroyer *> toDestroy;
+    if (m_dynamicRoles) {
+        for (int i=0 ; i < removeCount ; ++i) {
+            toDestroy.append(new ModelNodeDestoyer(m_modelObjects[index+i]));
+        }
+        m_modelObjects.remove(index, removeCount);
     } else {
-        qmlInfo(this) << tr("remove: incorrect number of arguments");
+        toDestroy = m_listModel->remove(index, removeCount);
     }
+
+    emitItemsRemoved(index, removeCount);
+    Q_FOREACH (ListModel::ElementDestroyer *destroyer, toDestroy)
+        delete destroyer;
 }
 
 /*!
diff --git a/src/qml/types/qqmllistmodel_p.h b/src/qml/types/qqmllistmodel_p.h
index b3ae806c5e..264749eab5 100644
--- a/src/qml/types/qqmllistmodel_p.h
+++ b/src/qml/types/qqmllistmodel_p.h
@@ -159,6 +159,8 @@ private:
     void emitItemsInserted(int index, int count);
     void emitItemsAboutToBeMoved(int from, int to, int n);
     void emitItemsMoved(int from, int to, int n);
+
+    void removeElements(int index, int removeCount);
 };
 
 // ### FIXME
diff --git a/src/qml/types/qqmllistmodel_p_p.h b/src/qml/types/qqmllistmodel_p_p.h
index f23fec1e59..983394e68b 100644
--- a/src/qml/types/qqmllistmodel_p_p.h
+++ b/src/qml/types/qqmllistmodel_p_p.h
@@ -356,8 +356,11 @@ public:
     int append(QV4::Object *object);
     void insert(int elementIndex, QV4::Object *object);
 
-    void clear();
-    void remove(int index, int count);
+    class ElementDestroyer {
+    public:
+        virtual ~ElementDestroyer() = 0;
+    };
+    Q_REQUIRED_RESULT QVector<ElementDestroyer *> remove(int index, int count);
 
     int appendElement();
     void insertElement(int index);