diff --git a/distribution/changelog.txt b/distribution/changelog.txt index 5071ba2467..0dacff9933 100644 --- a/distribution/changelog.txt +++ b/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). diff --git a/src/openrct2/rct1/S4Importer.cpp b/src/openrct2/rct1/S4Importer.cpp index 1fb200a892..b33ff0a31e 100644 --- a/src/openrct2/rct1/S4Importer.cpp +++ b/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(i); ImportEntity(gameState, _s4.Entities[i].Unknown); } } diff --git a/src/openrct2/rct2/S6Importer.cpp b/src/openrct2/rct2/S6Importer.cpp index fad4138650..fde0182e56 100644 --- a/src/openrct2/rct2/S6Importer.cpp +++ b/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(i); ImportEntity(gameState, _s6.Entities[i].Unknown); } } diff --git a/test/tests/CMakeLists.txt b/test/tests/CMakeLists.txt index 707bdf0434..483a54d1a5 100644 --- a/test/tests/CMakeLists.txt +++ b/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" diff --git a/test/tests/EntityImportTests.cpp b/test/tests/EntityImportTests.cpp new file mode 100644 index 0000000000..fabdbabc35 --- /dev/null +++ b/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 +#include +#include +#include +#include +#include +#include +#include +#include +#include + +using namespace OpenRCT2; + +class EntityImportTests : public testing::Test +{ +protected: + std::unique_ptr 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(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(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)); +} diff --git a/test/tests/testdata/parks/corrupted_duplicate_entity_indices.sv6 b/test/tests/testdata/parks/corrupted_duplicate_entity_indices.sv6 new file mode 100644 index 0000000000..46e72a87da Binary files /dev/null and b/test/tests/testdata/parks/corrupted_duplicate_entity_indices.sv6 differ diff --git a/test/tests/tests.vcxproj b/test/tests/tests.vcxproj index c79eab00b2..40cb31d587 100644 --- a/test/tests/tests.vcxproj +++ b/test/tests/tests.vcxproj @@ -95,6 +95,7 @@ +