diff options
author | Erik Verbruggen <erik.verbruggen@qt.io> | 2017-09-14 13:47:14 +0200 |
---|---|---|
committer | Erik Verbruggen <erik.verbruggen@qt.io> | 2017-09-14 11:54:45 +0000 |
commit | e29ffa179e9920443a23e2fcb3f0694df32e8a68 (patch) | |
tree | eb418119709f5f2e72a6331c310471cbc5828135 /src | |
parent | 3604cae410f29597c97ba73df6d31a9b54e6d30d (diff) |
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>
Diffstat (limited to 'src')
-rw-r--r-- | src/qml/types/qqmldelegatemodel.cpp | 7 | ||||
-rw-r--r-- | src/qml/types/qqmllistmodel.cpp | 25 | ||||
-rw-r--r-- | src/qml/types/qqmllistmodel_p_p.h | 2 |
3 files changed, 27 insertions, 7 deletions
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); |