From 7f75dc6dee8f3e7e34e0605187a5386868ad56aa Mon Sep 17 00:00:00 2001 From: Dennis Brakhane Date: Tue, 5 Oct 2021 02:41:35 +0200 Subject: [PATCH] Fix "index out of range" error in AssetProcessor (#4324) * Fix "index out of range" error When the parent is the tree root element, beginInsertRows must be called with an invalid (but legal) index. A QModelIndex with a row index of zero when the parent has no children is an illegal index and will result in "undefined behavior", like the "index out of range" one. Therefore, if our parent is the tree root element, we use QModelIndex() instead. Fixes #2343 Signed-off-by: Dennis Brakhane * Use QModelIndex() instead of createIndex(-1, -1) Both do the same, but the former is Qt best practise. Signed-off-by: Dennis Brakhane * add some sanity checks in debug mode Using illegal ModelIndices can result in hard to debug problems later on, so add a few checks to help spotting them sooner. Signed-off-by: Dennis Brakhane --- Code/Editor/UndoDropDown.cpp | 4 ++-- Code/Tools/AssetProcessor/native/ui/AssetTreeModel.cpp | 8 ++++++-- .../AssetProcessor/native/ui/ProductAssetTreeModel.cpp | 9 +++++++-- .../AssetProcessor/native/ui/SourceAssetTreeModel.cpp | 10 +++++++--- 4 files changed, 22 insertions(+), 9 deletions(-) diff --git a/Code/Editor/UndoDropDown.cpp b/Code/Editor/UndoDropDown.cpp index 6bf807ebf4..4f346b57dd 100644 --- a/Code/Editor/UndoDropDown.cpp +++ b/Code/Editor/UndoDropDown.cpp @@ -101,13 +101,13 @@ public: if (fresh.size() < m_stackNames.size()) { - beginRemoveRows(createIndex(-1, -1), static_cast(fresh.size()), static_cast(m_stackNames.size() - 1)); + beginRemoveRows(QModelIndex(), static_cast(fresh.size()), static_cast(m_stackNames.size() - 1)); m_stackNames = fresh; endRemoveRows(); } else { - beginInsertRows(createIndex(-1, -1), static_cast(m_stackNames.size()), static_cast(fresh.size() - 1)); + beginInsertRows(QModelIndex(), static_cast(m_stackNames.size()), static_cast(fresh.size() - 1)); m_stackNames = fresh; endInsertRows(); } diff --git a/Code/Tools/AssetProcessor/native/ui/AssetTreeModel.cpp b/Code/Tools/AssetProcessor/native/ui/AssetTreeModel.cpp index 4c2f168d4a..8d889343f9 100644 --- a/Code/Tools/AssetProcessor/native/ui/AssetTreeModel.cpp +++ b/Code/Tools/AssetProcessor/native/ui/AssetTreeModel.cpp @@ -168,7 +168,9 @@ namespace AssetProcessor if (childItem) { - return createIndex(row, column, childItem); + QModelIndex index = createIndex(row, column, childItem); + Q_ASSERT(checkIndex(index)); + return index; } return QModelIndex(); } @@ -197,7 +199,9 @@ namespace AssetProcessor { return QModelIndex(); } - return createIndex(parentItem->GetRow(), 0, parentItem); + QModelIndex parentIndex = createIndex(parentItem->GetRow(), 0, parentItem); + Q_ASSERT(checkIndex(parentIndex)); + return parentIndex; } bool AssetTreeModel::hasChildren(const QModelIndex &parent) const diff --git a/Code/Tools/AssetProcessor/native/ui/ProductAssetTreeModel.cpp b/Code/Tools/AssetProcessor/native/ui/ProductAssetTreeModel.cpp index 0574f00961..3f3a6b7a65 100644 --- a/Code/Tools/AssetProcessor/native/ui/ProductAssetTreeModel.cpp +++ b/Code/Tools/AssetProcessor/native/ui/ProductAssetTreeModel.cpp @@ -92,6 +92,7 @@ namespace AssetProcessor } QModelIndex parentIndex = createIndex(parent->GetRow(), 0, parent); + Q_ASSERT(checkIndex(parentIndex)); beginRemoveRows(parentIndex, assetToRemove->GetRow(), assetToRemove->GetRow()); @@ -179,6 +180,8 @@ namespace AssetProcessor QModelIndex existingIndexStart = createIndex(existingEntry->second->GetRow(), 0, existingEntry->second); QModelIndex existingIndexEnd = createIndex(existingEntry->second->GetRow(), existingEntry->second->GetColumnCount() - 1, existingEntry->second); + Q_ASSERT(checkIndex(existingIndexStart)); + Q_ASSERT(checkIndex(existingIndexEnd)); dataChanged(existingIndexStart, existingIndexEnd); return; } @@ -205,7 +208,8 @@ namespace AssetProcessor { if (!modelIsResetting) { - QModelIndex parentIndex = createIndex(parentItem->GetRow(), 0, parentItem); + QModelIndex parentIndex = parentItem == m_root.get() ? QModelIndex() : createIndex(parentItem->GetRow(), 0, parentItem); + Q_ASSERT(checkIndex(parentIndex)); beginInsertRows(parentIndex, parentItem->getChildCount(), parentItem->getChildCount()); } nextParent = parentItem->CreateChild(ProductAssetTreeItemData::MakeShared(nullptr, currentFullFolderPath.Native(), currentPath.c_str(), true, AZ::Uuid::CreateNull())); @@ -231,7 +235,8 @@ namespace AssetProcessor if (!modelIsResetting) { - QModelIndex parentIndex = createIndex(parentItem->GetRow(), 0, parentItem); + QModelIndex parentIndex = parentItem == m_root.get() ? QModelIndex() : createIndex(parentItem->GetRow(), 0, parentItem); + Q_ASSERT(checkIndex(parentIndex)); beginInsertRows(parentIndex, parentItem->getChildCount(), parentItem->getChildCount()); } diff --git a/Code/Tools/AssetProcessor/native/ui/SourceAssetTreeModel.cpp b/Code/Tools/AssetProcessor/native/ui/SourceAssetTreeModel.cpp index 88fbe5a204..eaebc57ca4 100644 --- a/Code/Tools/AssetProcessor/native/ui/SourceAssetTreeModel.cpp +++ b/Code/Tools/AssetProcessor/native/ui/SourceAssetTreeModel.cpp @@ -13,6 +13,7 @@ #include #include #include +#include namespace AssetProcessor { @@ -102,7 +103,8 @@ namespace AssetProcessor { if (!modelIsResetting) { - QModelIndex parentIndex = createIndex(parentItem->GetRow(), 0, parentItem); + QModelIndex parentIndex = parentItem == m_root.get() ? QModelIndex() : createIndex(parentItem->GetRow(), 0, parentItem); + Q_ASSERT(checkIndex(parentIndex)); beginInsertRows(parentIndex, parentItem->getChildCount(), parentItem->getChildCount()); } nextParent = parentItem->CreateChild(SourceAssetTreeItemData::MakeShared(nullptr, nullptr, currentFullFolderPath.Native(), currentPath.c_str(), true)); @@ -118,7 +120,8 @@ namespace AssetProcessor if (!modelIsResetting) { - QModelIndex parentIndex = createIndex(parentItem->GetRow(), 0, parentItem); + QModelIndex parentIndex = parentItem == m_root.get() ? QModelIndex() : createIndex(parentItem->GetRow(), 0, parentItem); + Q_ASSERT(checkIndex(parentIndex)); beginInsertRows(parentIndex, parentItem->getChildCount(), parentItem->getChildCount()); } @@ -173,7 +176,8 @@ namespace AssetProcessor return; } - QModelIndex parentIndex = createIndex(parent->GetRow(), 0, parent); + QModelIndex parentIndex = parent == m_root.get() ? QModelIndex() : createIndex(parent->GetRow(), 0, parent); + Q_ASSERT(checkIndex(parentIndex)); beginRemoveRows(parentIndex, assetToRemove->GetRow(), assetToRemove->GetRow());