Skip to content

Commit

Permalink
Fix use-after-free when clear()ing all elements from a ListModel
Browse files Browse the repository at this point in the history
Same problem as the problem with remove(), so now clear will call into
remove to do the correct thing.

See also e29ffa1.

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

Change-Id: Ib9389d80798c4333425b4a49930b1670307d06ac
Task-number: QTBUG-59256
Reviewed-by: Simon Hausmann <simon.hausmann@qt.io>
  • Loading branch information
Erik Verbruggen committed Sep 22, 2017
1 parent 221b3f6 commit 163c515
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 41 deletions.
66 changes: 26 additions & 40 deletions src/qml/types/qqmllistmodel.cpp
Expand Up @@ -341,7 +341,9 @@ ListModel::ListModel(ListLayout *layout, QQmlListModel *modelCache, int uid) : m

void ListModel::destroy()
{
clear();
for (const auto &destroyer : remove(0, elements.count()))
destroyer();

m_uid = -1;
m_layout = 0;
if (m_modelCache && m_modelCache->m_primary == false)
Expand Down Expand Up @@ -557,16 +559,6 @@ void ListModel::set(int elementIndex, QV4::Object *object)
}
}

void ListModel::clear()
{
int elementCount = elements.count();
for (int i=0 ; i < elementCount ; ++i) {
elements[i]->destroy(m_layout);
delete elements[i];
}
elements.clear();
}

QVector<std::function<void()>> ListModel::remove(int index, int count)
{
QVector<std::function<void()>> toDestroy;
Expand Down Expand Up @@ -2025,18 +2017,7 @@ int QQmlListModel::count() const
*/
void QQmlListModel::clear()
{
const int cleared = count();

emitItemsAboutToBeRemoved(0, cleared);

if (m_dynamicRoles) {
qDeleteAll(m_modelObjects);
m_modelObjects.clear();
} else {
m_listModel->clear();
}

emitItemsRemoved(0, cleared);
removeElements(0, count());
}

/*!
Expand All @@ -2060,27 +2041,32 @@ void QQmlListModel::remove(QQmlV4Function *args)
return;
}

emitItemsAboutToBeRemoved(index, removeCount);
removeElements(index, removeCount);
} else {
qmlWarning(this) << tr("remove: incorrect number of arguments");
}
}

QVector<std::function<void()>> toDestroy;
if (m_dynamicRoles) {
for (int i=0 ; i < removeCount ; ++i) {
auto modelObject = m_modelObjects[index+i];
toDestroy.append([modelObject](){
delete modelObject;
});
}
m_modelObjects.remove(index, removeCount);
} else {
toDestroy = m_listModel->remove(index, removeCount);
}
void QQmlListModel::removeElements(int index, int removeCount)
{
emitItemsAboutToBeRemoved(index, removeCount);

emitItemsRemoved(index, removeCount);
for (const auto &destroyer : toDestroy)
destroyer();
QVector<std::function<void()>> toDestroy;
if (m_dynamicRoles) {
for (int i=0 ; i < removeCount ; ++i) {
auto modelObject = m_modelObjects[index+i];
toDestroy.append([modelObject](){
delete modelObject;
});
}
m_modelObjects.remove(index, removeCount);
} else {
qmlWarning(this) << tr("remove: incorrect number of arguments");
toDestroy = m_listModel->remove(index, removeCount);
}

emitItemsRemoved(index, removeCount);
for (const auto &destroyer : toDestroy)
destroyer();
}

/*!
Expand Down
2 changes: 2 additions & 0 deletions src/qml/types/qqmllistmodel_p.h
Expand Up @@ -165,6 +165,8 @@ class Q_QML_PRIVATE_EXPORT QQmlListModel : public QAbstractListModel
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
Expand Down
1 change: 0 additions & 1 deletion src/qml/types/qqmllistmodel_p_p.h
Expand Up @@ -366,7 +366,6 @@ class ListModel
int append(QV4::Object *object);
void insert(int elementIndex, QV4::Object *object);

void clear();
Q_REQUIRED_RESULT QVector<std::function<void()>> remove(int index, int count);

int appendElement();
Expand Down

0 comments on commit 163c515

Please sign in to comment.