QAbstractItemView: do not access invalid model indices (3/N)

The slots connected to rowsAboutToBeRemoved and columnsAboutToBeRemoved
attempt to find a new current index by scanning the model around the
existing current index -- in case that index is indeed about to be
removed.

The problem is that the scanning was done without any sorts of bounds
checking; instead it was relying on the model's index() to return an
invalid index for an out of bounds request.

Fix that by adding bounds checking. Since models are not supposed
to return invalid indices for in-bounds index() calls, added some
warnings if that happens.

For some reason, the code handling rows and columns isn't perfectly
symmetrical. Rows are searched both forwards and backwards, while
columns only backwards, and the related code is slightly different.
Filed QTBUG-100273 to try and understand what's going on.

Change-Id: I7452d8c521e74daa4408e6cc969ce5a6059f53ea
Pick-to: 5.15 6.2 6.3
Reviewed-by: David Faure <david.faure@kdab.com>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>

1 files changed, 41 insertions, 7 deletions

diff --git a/src/widgets/itemviews/qabstractitemview.cpp b/src/widgets/itemviews/qabstractitemview.cpp
index ae03f27fac..21d0cf2e2c 100644
--- a/src/widgets/itemviews/qabstractitemview.cpp
+++ b/src/widgets/itemviews/qabstractitemview.cpp
@@ -3477,15 +3477,39 @@ void QAbstractItemView::rowsAboutToBeRemoved(const QModelIndex &parent, int star
         } else {
             int row = end + 1;
             QModelIndex next;
-            do { // find the next visible and enabled item
+            const int rowCount = d->model->rowCount(parent);
+            bool found = false;
+            // find the next visible and enabled item
+            while (row < rowCount && !found) {
                 next = d->model->index(row++, current.column(), current.parent());
-            } while (next.isValid() && (isIndexHidden(next) || !d->isIndexEnabled(next)));
-            if (row > d->model->rowCount(parent)) {
+#ifdef QT_DEBUG
+                if (!next.isValid()) {
+                    qWarning("Model unexpectedly returned an invalid index");
+                    break;
+                }
+#endif
+                if (!isIndexHidden(next) && d->isIndexEnabled(next)) {
+                    found = true;
+                    break;
+                }
+            }
+
+            if (!found) {
                 row = start - 1;
-                do { // find the previous visible and enabled item
+                // find the previous visible and enabled item
+                while (row >= 0) {
                     next = d->model->index(row--, current.column(), current.parent());
-                } while (next.isValid() && (isIndexHidden(next) || !d->isIndexEnabled(next)));
+#ifdef QT_DEBUG
+                    if (!next.isValid()) {
+                        qWarning("Model unexpectedly returned an invalid index");
+                        break;
+                    }
+#endif
+                    if (!isIndexHidden(next) && d->isIndexEnabled(next))
+                        break;
+                }
             }
+
             setCurrentIndex(next);
         }
     }
@@ -3563,9 +3587,19 @@ void QAbstractItemViewPrivate::_q_columnsAboutToBeRemoved(const QModelIndex &par
         } else {
             int column = end;
             QModelIndex next;
-            do { // find the next visible and enabled item
+            const int columnCount = model->columnCount(current.parent());
+            // find the next visible and enabled item
+            while (column < columnCount) {
                 next = model->index(current.row(), column++, current.parent());
-            } while (next.isValid() && (q->isIndexHidden(next) || !isIndexEnabled(next)));
+#ifdef QT_DEBUG
+                if (!next.isValid()) {
+                    qWarning("Model unexpectedly returned an invalid index");
+                    break;
+                }
+#endif
+                if (!q->isIndexHidden(next) && isIndexEnabled(next))
+                    break;
+            }
             q->setCurrentIndex(next);
         }
     }