From 47b54cd3ad35201ff2ab9ef6bfdba83fc086558d Mon Sep 17 00:00:00 2001 From: Shane Synan Date: Wed, 15 Jul 2020 19:08:41 -0400 Subject: [PATCH] uisupport: Fix invalid model segfault from index The Qt implementation of QModelIndex::child(...) automatically checks if the QAbstractItemModel is valid, and if not, it returns an invalid QModelIndex. Quassel relied upon this in several places, checking if the QModelIndex was valid without checking the QAbstractItemModel itself, which introduced crashes upon migrating away from the deprecated QModelIndex::child(...) function. Specifically... #if QT_DEPRECATED_SINCE(5, 8) inline QModelIndex QModelIndex::child(int arow, int acolumn) const { return m ? m->index(arow, acolumn, *this) : QModelIndex(); } #endif To address this, check the QModelIndex's model to verify it's valid wherever Quassel had previously relied upon a check to QModelIndex::isValid() to ensure validity. Places without a validity check aren't adjusted under the assumption it won't ever be an issue. Follow-up to the Qt deprecation fixes in a453c963cf1872e14c83adf1d40a31821c166805 (It's unfortunate that Qt's deprecation warning mentioned this, but only in the detailed documentation and not as clearly as expected: https://doc.qt.io/qt-5/qmodelindex-obsolete.html ) Special thanks to oprypin for reporting this with a detailed, reproducible test case! Fixes #1583 --- src/client/treemodel.cpp | 12 ++++++++++++ src/qtui/nicklistwidget.cpp | 4 ++++ src/uisupport/abstractbuffercontainer.cpp | 4 ++++ src/uisupport/bufferview.cpp | 16 +++++++++++----- src/uisupport/nickviewfilter.cpp | 2 +- 5 files changed, 32 insertions(+), 6 deletions(-) diff --git a/src/client/treemodel.cpp b/src/client/treemodel.cpp index 8d6c8824..006434c8 100644 --- a/src/client/treemodel.cpp +++ b/src/client/treemodel.cpp @@ -569,6 +569,12 @@ void TreeModel::debug_rowsAboutToBeRemoved(const QModelIndex& parent, int start, parentItem = rootItem; qDebug() << "debug_rowsAboutToBeRemoved" << parent << parentItem << parent.data().toString() << rowCount(parent) << start << end; + // Make sure model is valid first + if (!parent.model()) { + qDebug() << "Parent model is not valid!" << end; + return; + } + QModelIndex child; for (int i = end; i >= start; i--) { child = parent.model()->index(i, 0, parent); @@ -585,6 +591,12 @@ void TreeModel::debug_rowsInserted(const QModelIndex& parent, int start, int end parentItem = rootItem; qDebug() << "debug_rowsInserted:" << parent << parentItem << parent.data().toString() << rowCount(parent) << start << end; + // Make sure model is valid first + if (!parent.model()) { + qDebug() << "Parent model is not valid!" << end; + return; + } + QModelIndex child; for (int i = start; i <= end; i++) { child = parent.model()->index(i, 0, parent); diff --git a/src/qtui/nicklistwidget.cpp b/src/qtui/nicklistwidget.cpp index e11d312b..5ab410b3 100644 --- a/src/qtui/nicklistwidget.cpp +++ b/src/qtui/nicklistwidget.cpp @@ -176,6 +176,10 @@ void NickListWidget::rowsAboutToBeRemoved(const QModelIndex& parent, int start, } else { // check if there are explicitly buffers removed + // Make sure model is valid first + if (!parent.model()) { + return; + } for (int i = start; i <= end; i++) { QVariant variant = parent.model()->index(i, 0, parent).data(NetworkModel::BufferIdRole); if (!variant.isValid()) diff --git a/src/uisupport/abstractbuffercontainer.cpp b/src/uisupport/abstractbuffercontainer.cpp index ca066d0b..922e2e9b 100644 --- a/src/uisupport/abstractbuffercontainer.cpp +++ b/src/uisupport/abstractbuffercontainer.cpp @@ -46,6 +46,10 @@ void AbstractBufferContainer::rowsAboutToBeRemoved(const QModelIndex& parent, in } else { // check if there are explicitly buffers removed + // Make sure model is valid first + if (!parent.model()) { + return; + } for (int i = start; i <= end; i++) { QVariant variant = parent.model()->index(i, 0, parent).data(NetworkModel::BufferIdRole); if (!variant.isValid()) diff --git a/src/uisupport/bufferview.cpp b/src/uisupport/bufferview.cpp index 4af58a3d..f5db3e29 100644 --- a/src/uisupport/bufferview.cpp +++ b/src/uisupport/bufferview.cpp @@ -493,14 +493,18 @@ void BufferView::changeBuffer(Direction direction) QModelIndex newParent = currentIndex.sibling(currentIndex.row() - 1, 0); if (currentIndex.row() == 0) newParent = lastNetIndex; - if (model()->hasChildren(newParent)) - resultingIndex = newParent.model()->index(model()->rowCount(newParent) - 1, 0, newParent); + if (model()->hasChildren(newParent)) { + // Treat an invalid QAbstractItemModel as an invalid QModelIndex + resultingIndex = (newParent.model() ? newParent.model()->index(model()->rowCount(newParent) - 1, 0, newParent) : QModelIndex()); + } else resultingIndex = newParent; } else { - if (model()->hasChildren(currentIndex)) - resultingIndex = currentIndex.model()->index(0, 0, currentIndex); + if (model()->hasChildren(currentIndex)) { + // Treat an invalid QAbstractItemModel as an invalid QModelIndex + resultingIndex = (currentIndex.model() ? currentIndex.model()->index(0, 0, currentIndex) : QModelIndex()); + } else resultingIndex = currentIndex.sibling(currentIndex.row() + 1, 0); } @@ -509,8 +513,10 @@ void BufferView::changeBuffer(Direction direction) if (!resultingIndex.isValid()) { if (direction == Forward) resultingIndex = model()->index(0, 0, QModelIndex()); - else + else { + // Assume model is valid resultingIndex = lastNetIndex.model()->index(model()->rowCount(lastNetIndex) - 1, 0, lastNetIndex); + } } selectionModel()->setCurrentIndex(resultingIndex, QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows); diff --git a/src/uisupport/nickviewfilter.cpp b/src/uisupport/nickviewfilter.cpp index b48d0c83..9e480fe9 100644 --- a/src/uisupport/nickviewfilter.cpp +++ b/src/uisupport/nickviewfilter.cpp @@ -41,7 +41,7 @@ NickViewFilter::NickViewFilter(const BufferId& bufferId, NetworkModel* parent) bool NickViewFilter::filterAcceptsRow(int source_row, const QModelIndex& source_parent) const { // root node, networkindexes, the bufferindex of the buffer this filter is active for and it's children are accepted - if (!source_parent.isValid()) + if (!(source_parent.isValid() && source_parent.model())) return true; QModelIndex source_child = source_parent.model()->index(source_row, 0, source_parent); -- 2.20.1