QSqlTableModel: fix segfault when overriding selectRow()

The STL-style iteration over the cache in submitAll() assumed the
iterator would remain valid until reaching cache.end(). This failed
to consider that virtual selectRow() might be overridden so that
it removes rows from the cache. For example, it might call select()
which would empty the cache.

The new approach checks at each iteration whether the row is
still in the cache. Using foreach here is justified by its fitness
for purpose and readability.

New test included.

Change-Id: Idee8807ede239c3ba56ff1604574c49f47385ad2
Reviewed-by: David Faure (fixes for KDE) <faure@kde.org>

diff --git a/src/sql/models/qsqltablemodel.cpp b/src/sql/models/qsqltablemodel.cpp
index 95cd0d8..56838a4 100644 (file)
--- a/src/sql/models/qsqltablemodel.cpp
+++ b/src/sql/models/qsqltablemodel.cpp
@@ -718,20 +718,25 @@ bool QSqlTableModel::submitAll()
 
     bool success = true;
 
-    for (QSqlTableModelPrivate::CacheMap::Iterator it = d->cache.begin();
-         it != d->cache.end(); ++it) {
-        if (it.value().submitted())
+    foreach (int row, d->cache.keys()) {
+        // be sure cache *still* contains the row since overriden selectRow() could have called select()
+        QSqlTableModelPrivate::CacheMap::iterator it = d->cache.find(row);
+        if (it == d->cache.end())
             continue;
 
-        switch (it.value().op()) {
+        QSqlTableModelPrivate::ModifiedRow &mrow = it.value();
+        if (mrow.submitted())
+            continue;
+
+        switch (mrow.op()) {
         case QSqlTableModelPrivate::Insert:
-            success = insertRowIntoTable(it.value().rec());
+            success = insertRowIntoTable(mrow.rec());
             break;
         case QSqlTableModelPrivate::Update:
-            success = updateRowInTable(it.key(), it.value().rec());
+            success = updateRowInTable(row, mrow.rec());
             break;
         case QSqlTableModelPrivate::Delete:
-            success = deleteRowFromTable(it.key());
+            success = deleteRowFromTable(row);
             break;
         case QSqlTableModelPrivate::None:
             Q_ASSERT_X(false, "QSqlTableModel::submitAll()", "Invalid cache operation");
@@ -739,9 +744,9 @@ bool QSqlTableModel::submitAll()
         }
 
         if (success) {
-            it.value().setSubmitted();
+            mrow.setSubmitted();
             if (d->strategy != OnManualSubmit)
-                success = selectRow(it.key());
+                success = selectRow(row);
         }
 
         if (!success)

diff --git a/tests/auto/sql/models/qsqltablemodel/tst_qsqltablemodel.cpp b/tests/auto/sql/models/qsqltablemodel/tst_qsqltablemodel.cpp
index cbc9cd5..a4e1fa1 100644 (file)
--- a/tests/auto/sql/models/qsqltablemodel/tst_qsqltablemodel.cpp
+++ b/tests/auto/sql/models/qsqltablemodel/tst_qsqltablemodel.cpp
@@ -77,6 +77,8 @@ private slots:
     void select();
     void selectRow_data() { generic_data(); }
     void selectRow();
+    void selectRowOverride_data() { generic_data(); }
+    void selectRowOverride();
     void insertColumns_data() { generic_data_with_strategies(); }
     void insertColumns();
     void submitAll_data() { generic_data(); }
@@ -356,6 +358,53 @@ void tst_QSqlTableModel::selectRow()
     q.exec("DELETE FROM " + tbl);
 }
 
+class SelectRowOverrideTestModel: public QSqlTableModel
+{
+    Q_OBJECT
+public:
+    SelectRowOverrideTestModel(QObject *parent, QSqlDatabase db):QSqlTableModel(parent, db) { }
+    bool selectRow(int row)
+    {
+        Q_UNUSED(row)
+        return select();
+    }
+};
+
+void tst_QSqlTableModel::selectRowOverride()
+{
+    QFETCH(QString, dbName);
+    QSqlDatabase db = QSqlDatabase::database(dbName);
+    CHECK_DATABASE(db);
+
+    QString tbl = qTableName("pktest", __FILE__);
+    QSqlQuery q(db);
+    q.exec("DELETE FROM " + tbl);
+    q.exec("INSERT INTO " + tbl + " (id, a) VALUES (0, 'a')");
+    q.exec("INSERT INTO " + tbl + " (id, a) VALUES (1, 'b')");
+    q.exec("INSERT INTO " + tbl + " (id, a) VALUES (2, 'c')");
+
+    SelectRowOverrideTestModel model(0, db);
+    model.setEditStrategy(QSqlTableModel::OnFieldChange);
+    model.setTable(tbl);
+    model.setSort(0, Qt::AscendingOrder);
+    QVERIFY_SQL(model, select());
+
+    QCOMPARE(model.rowCount(), 3);
+    QCOMPARE(model.columnCount(), 2);
+
+    q.exec("UPDATE " + tbl + " SET a = 'Qt' WHERE id = 2");
+    QModelIndex idx = model.index(1, 1);
+    // overridden selectRow() should select() whole table and not crash
+    model.setData(idx, QString("Qt"));
+
+    // both rows should have changed
+    QCOMPARE(model.data(idx).toString(), QString("Qt"));
+    idx = model.index(2, 1);
+    QCOMPARE(model.data(idx).toString(), QString("Qt"));
+
+    q.exec("DELETE FROM " + tbl);
+}
+
 void tst_QSqlTableModel::insertColumns()
 {
     // Just like the select test, with extra stuff