[Scummvm-git-logs] scummvm master -> 2fbeecab23f41eaace8ea26e9021022aa2d08cf1

[sluicebox]

This automated email contains information about 1 new commit which have been
pushed to the 'scummvm' repo located at https://github.com/scummvm/scummvm .

Summary:
2fbeecab23 SCI: Prevent script lockers from overflowing


Commit: 2fbeecab23f41eaace8ea26e9021022aa2d08cf1
    https://github.com/scummvm/scummvm/commit/2fbeecab23f41eaace8ea26e9021022aa2d08cf1
Author: sluicebox (22204938+sluicebox at users.noreply.github.com)
Date: 2022-11-22T12:37:32-08:00

Commit Message:
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.

Changed paths:
    engines/sci/engine/savegame.cpp
    engines/sci/engine/script.cpp
    engines/sci/engine/script.h


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<byte> _script; /**< Script size includes alignment byte */
 	SciSpan<byte> _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<const uint16> _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