Fix #25963: Crash when loading corrupted saves with duplicate entity indices (#25980)

Corrupted RCT1/RCT2 save files may contain entities with duplicate or invalid EntityIndex values and when importing these saves, CreateEntityAt returns nullptr for duplicate indices, causing a null pointer dereference crash in ImportEntityCommonProperties, this sanitises each entity's (entities?) EntityIndex to match its array position, so that CreateEntityAt always succeeds. Added unit tests too for just this case, with a crashing save file.

distribution/changelog.txt

@@ -4,6 +4,7 @@
 - Improved: [#21753] Tracked rides with cheated powered launch mode can change powered launch speed without extra cheats.
 - Improved: [#25941] The command line sprite build command is now faster.
 - Fix: [#14686, #23996, #25981] Preview images from different windows overwrite each other when using OpenGL.
+- Fix: [#15128, #15626, #16331, #17443, #18626, #21597, #24175, #24971, #25963] Crash when loading corrupted RCT2/RCT1 save files with duplicate entity indices.
 - Fix: [#25237] Wrong colours on the Knight costume.
 - Fix: [#25712, #25727] Crash during startup from window list race condition.
 - Fix: [#25910] Folders starting with numbers are sorted incorrectly (e.g. 1, 10, 2 instead of 1, 2, 10).

src/openrct2/rct1/S4Importer.cpp

@@ -1245,6 +1245,9 @@ namespace OpenRCT2::RCT1
         {
             for (int i = 0; i < Limits::kMaxEntities; i++)
             {
+                // Make sure the EntityIndex matches the array position to handle corrupted saves where duplicate or invalid
+                // indices would cause CreateEntityAt to fail
+                _s4.Entities[i].Unknown.EntityIndex = static_cast<uint16_t>(i);
                 ImportEntity(gameState, _s4.Entities[i].Unknown);
             }
         }

src/openrct2/rct2/S6Importer.cpp

@@ -1651,6 +1651,9 @@ namespace OpenRCT2::RCT2
         {
             for (int32_t i = 0; i < GetMaxEntities(); i++)
             {
+                // Make sure the EntityIndex matches the array position to handle corrupted saves where duplicate or invalid
+                // indices would cause CreateEntityAt to fail
+                _s6.Entities[i].Unknown.EntityIndex = static_cast<uint16_t>(i);
                 ImportEntity(gameState, _s6.Entities[i].Unknown);
             }
         }

test/tests/CMakeLists.txt

@@ -26,6 +26,7 @@ set(test_files
    "${CMAKE_CURRENT_SOURCE_DIR}/PlayTests.cpp"
    "${CMAKE_CURRENT_SOURCE_DIR}/ReplayTests.cpp"
    "${CMAKE_CURRENT_SOURCE_DIR}/RideRatings.cpp"
+   "${CMAKE_CURRENT_SOURCE_DIR}/EntityImportTests.cpp"
    "${CMAKE_CURRENT_SOURCE_DIR}/S6ImportExportTests.cpp"
    "${CMAKE_CURRENT_SOURCE_DIR}/SawyerCodingTest.cpp"
    "${CMAKE_CURRENT_SOURCE_DIR}/ScenarioPatcherTests.cpp"

test/tests/EntityImportTests.cpp

@@ -0,0 +1,79 @@
+/*****************************************************************************
+ * Copyright (c) 2014-2026 OpenRCT2 developers
+ *
+ * For a complete list of all authors, please refer to contributors.md
+ * Interested in contributing? Visit https://github.com/OpenRCT2/OpenRCT2
+ *
+ * OpenRCT2 is licensed under the GNU General Public License version 3.
+ *****************************************************************************/
+
+#include "TestData.h"
+
+#include <gtest/gtest.h>
+#include <openrct2/Context.h>
+#include <openrct2/GameState.h>
+#include <openrct2/OpenRCT2.h>
+#include <openrct2/ParkImporter.h>
+#include <openrct2/core/FileStream.h>
+#include <openrct2/entity/EntityRegistry.h>
+#include <openrct2/entity/Guest.h>
+#include <openrct2/object/ObjectManager.h>
+#include <openrct2/world/MapAnimation.h>
+
+using namespace OpenRCT2;
+
+class EntityImportTests : public testing::Test
+{
+protected:
+    std::unique_ptr<IContext> context;
+
+    void SetUp() override
+    {
+        gOpenRCT2Headless = true;
+        gOpenRCT2NoGraphics = true;
+        context = CreateContext();
+        ASSERT_NE(context, nullptr);
+        ASSERT_TRUE(context->Initialise());
+    }
+
+    void TearDown() override
+    {
+        context.reset();
+    }
+};
+
+// This here tests that CreateEntityAt returns nullptr when trying to create an entity at an index that's already occupied.
+// This behavior is what caused crashes before, corrupted saves had duplicate EntityIndex values that caused CreateEntityAt to
+// return nullptr, which was then dereferenced.
+TEST_F(EntityImportTests, CreateEntityAtDuplicateIndexReturnsNull)
+{
+    auto& gameState = getGameState();
+    gameState.entities.ResetAllEntities();
+
+    // Create an entity at index 100
+    auto* entity1 = gameState.entities.CreateEntityAt<Guest>(EntityId::FromUnderlying(100));
+    ASSERT_NE(entity1, nullptr);
+    EXPECT_EQ(entity1->Id.ToUnderlying(), 100u);
+
+    // Try to create another entity at the same index, which should return nullptr
+    auto* entity2 = gameState.entities.CreateEntityAt<Guest>(EntityId::FromUnderlying(100));
+    EXPECT_EQ(entity2, nullptr);
+}
+
+// This test verifies that corrupted S6 files with duplicate EntityIndex values can be loaded without crashing.
+TEST_F(EntityImportTests, S6ImportCorruptedDuplicateEntityIndicesDoesNotCrash)
+{
+    std::string testParkPath = TestData::GetParkPath("corrupted_duplicate_entity_indices.sv6");
+    auto fs = FileStream(testParkPath, FileMode::open);
+
+    auto& objManager = context->GetObjectManager();
+    auto importer = ParkImporter::CreateS6(context->GetObjectRepository());
+    auto loadResult = importer->LoadFromStream(&fs, false);
+    objManager.LoadObjects(loadResult.RequiredObjects);
+
+    MapAnimations::ClearAll();
+    auto& gameState = getGameState();
+
+    // This should not crash because the code should sanitize EntityIndex values before CreateEntityAt is called
+    EXPECT_NO_THROW(importer->Import(gameState));
+}

test/tests/tests.vcxproj

@@ -95,6 +95,7 @@
     <ClCompile Include="PlayTests.cpp" />
     <ClCompile Include="Pathfinding.cpp" />
     <ClCompile Include="RideRatings.cpp" />
+    <ClCompile Include="EntityImportTests.cpp" />
     <ClCompile Include="S6ImportExportTests.cpp" />
     <ClCompile Include="SawyerCodingTest.cpp" />
     <ClCompile Include="ScenarioPatcherTests.cpp" />