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

diff --git a/src/client/treemodel.cpp b/src/client/treemodel.cpp
index 8d6c882..006434c 100644 (file)
--- 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 e11d312..5ab410b 100644 (file)
--- 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 ca066d0..922e2e9 100644 (file)
--- 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 4af58a3..f5db3e2 100644 (file)
--- 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 b48d0c8..9e480fe 100644 (file)
--- 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);