diff options
author | Erik Verbruggen <erik.verbruggen@qt.io> | 2017-09-14 13:47:14 +0200 |
---|---|---|
committer | Simon Hausmann <simon.hausmann@qt.io> | 2018-02-22 08:22:31 +0000 |
commit | 942b7935107f651b62bd77d6ec62adb7fd0fe0d1 (patch) | |
tree | 7c49466f880dd8a0a4f7926cb5940c4f58fe46e5 /src | |
parent | f8edd75b47cb0d5fe555b44771f575d581c6acca (diff) |
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>
Diffstat (limited to 'src')
-rw-r--r-- | src/qml/types/qqmldelegatemodel.cpp | 7 | ||||
-rw-r--r-- | src/qml/types/qqmllistmodel.cpp | 91 | ||||
-rw-r--r-- | src/qml/types/qqmllistmodel_p.h | 2 | ||||
-rw-r--r-- | src/qml/types/qqmllistmodel_p_p.h | 7 |
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); |