From 2fbeecab23f41eaace8ea26e9021022aa2d08cf1 Mon Sep 17 00:00:00 2001 From: sluicebox <22204938+sluicebox@users.noreply.github.com> Date: Tue, 22 Nov 2022 12:37:32 -0800 Subject: [PATCH] SCI: Prevent script lockers from overflowing Script::_locker is supposed to track the number of classes and objects that use a script in order to determine when it can be safely unloaded. But for system script 999, _locker rapidly increases throughout the game and eventually overflows. This causes script 999 to unload and crash. Other scripts may also be affected. We have been provided QFG1 SCI0 saves where this occurred over the course of hours within only three days. Logging shows this value skyrocketing in many games even when they are just polling input or animating the screen. Alarmingly, this suggests that many SCI games become unplayable in ScummVM after an achievable amount of game time. This may be a large and complex problem within the script locker system. For now, we can mitigate this by making Script::_locker unsigned, which its callers expected it to be, and by logging overflow attempts and lowering the locker. This fixes existing save games, even if they have already overflowed the signed maximum. --- engines/sci/engine/savegame.cpp | 2 +- engines/sci/engine/script.cpp | 19 +++++++++++++++++-- engines/sci/engine/script.h | 6 +++--- 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/engines/sci/engine/savegame.cpp b/engines/sci/engine/savegame.cpp index a27b699dd1a..e67e4764a2d 100644 --- a/engines/sci/engine/savegame.cpp +++ b/engines/sci/engine/savegame.cpp @@ -568,7 +568,7 @@ void Script::saveLoadWithSerializer(Common::Serializer &s) { s.skip(4, VER(14), VER(19)); // OBSOLETE: Used to be _numExports s.skip(4, VER(14), VER(19)); // OBSOLETE: Used to be _numSynonyms - s.syncAsSint32LE(_lockers); + s.syncAsUint32LE(_lockers); // Sync _objects. This is a hashmap, and we use the following on disk format: // First we store the number of items in the hashmap, then we store each diff --git a/engines/sci/engine/script.cpp b/engines/sci/engine/script.cpp index a1ba52184d8..cbc91293530 100644 --- a/engines/sci/engine/script.cpp +++ b/engines/sci/engine/script.cpp @@ -841,6 +841,21 @@ void Script::relocateSci3(const SegmentId segmentId) { void Script::incrementLockers() { assert(!_markedAsDeleted); + if (_lockers == 0xffffffff) { + // FIXME + // The locker on system scripts, most notably script 999, increases throughout + // the game and can eventually overflow, causing script 999 to be unloaded + // and crash. We have been provided QFG1 SCI0 save games where this occurred + // within hours of play over the course of three days. Debugging shows that + // this value increases rapidly, for example: on every `class Event` opcode + // during input polling, or when our kAnimate code calls `doit` on objects. + // Until this is fixed properly, log these overflow attempts and lower the + // counter so that the warnings continue without flooding the log. + // At the point where a resource usage counter reaches the billions, its value + // no longer has meaning, so the important thing for now is that it not crash. + warning("script %d locker maximum reached, resetting", _nr); + _lockers = 0x80000000; + } _lockers++; } @@ -849,11 +864,11 @@ void Script::decrementLockers() { _lockers--; } -int Script::getLockers() const { +uint Script::getLockers() const { return _lockers; } -void Script::setLockers(int lockers) { +void Script::setLockers(uint lockers) { assert(lockers == 0 || !_markedAsDeleted); _lockers = lockers; } diff --git a/engines/sci/engine/script.h b/engines/sci/engine/script.h index 4f9e086daf1..13c441d071c 100644 --- a/engines/sci/engine/script.h +++ b/engines/sci/engine/script.h @@ -75,7 +75,7 @@ private: SciSpan _script; /**< Script size includes alignment byte */ SciSpan _heap; /**< Start of heap if SCI1.1, NULL otherwise */ - int _lockers; /**< Number of classes and objects that require this script */ + uint _lockers; /**< Number of classes and objects that require this script */ SciSpan _exports; /**< Exports block or 0 if not present */ uint16 _numExports; /**< Number of export entries */ @@ -197,10 +197,10 @@ public: * Retrieves the number of locks held on this script. * @return the number of locks held on the previously identified script */ - int getLockers() const; + uint getLockers() const; /** Sets the number of locks held on this script. */ - void setLockers(int lockers); + void setLockers(uint lockers); /** * Retrieves the offset of the export table in the script