[Scummvm-git-logs] scummvm master -> 879f6a43f5e61d3f745a775870c0e7da6ce23b7b

[athrxx]

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:
879f6a43f5 AGOS: fix timer handling in savegames (bug no. 14886)


Commit: 879f6a43f5e61d3f745a775870c0e7da6ce23b7b
    https://github.com/scummvm/scummvm/commit/879f6a43f5e61d3f745a775870c0e7da6ce23b7b
Author: athrxx (athrxx at scummvm.org)
Date: 2024-04-04T22:12:01+02:00

Commit Message:
AGOS: fix timer handling in savegames (bug no. 14886)

 ("Waxworks crashing at Egypt Level 3, corrupting save file")

The design for the timers is a bit sloppy, even the original
interpreter often writes negative timeouts into the save files.
We replicated this correctly, but treated these values as very
large positives which effectively disabled the timers.

The bug report is only about Waxworks, but I checked Elvira
and i have the same absurd timer values in my old savegames
there. It seems that the timers have little impact in most cases,
but in Waxworks there is a timer that runs every 10 seconds
which cleans up the items chain. And if that doesn't happen
it will lead to invalid item handles, like in that bug ticket.

Changed paths:
    engines/agos/agos.h
    engines/agos/event.cpp
    engines/agos/saveload.cpp


diff --git a/engines/agos/agos.h b/engines/agos/agos.h
index d8ee88da835..6c3a154dc20 100644
--- a/engines/agos/agos.h
+++ b/engines/agos/agos.h
@@ -733,7 +733,7 @@ protected:
 	const byte *getLocalStringByID(uint16 stringId);
 	uint getNextStringID();
 
-	void addTimeEvent(uint16 timeout, uint16 subroutineId);
+	void addTimeEvent(int32 timeout, uint16 subroutineId);
 	void delTimeEvent(TimeEvent *te);
 
 	Item *findInByClass(Item *i, int16 m);
diff --git a/engines/agos/event.cpp b/engines/agos/event.cpp
index e8595eb8041..ffb1c8dea96 100644
--- a/engines/agos/event.cpp
+++ b/engines/agos/event.cpp
@@ -38,7 +38,7 @@
 
 namespace AGOS {
 
-void AGOSEngine::addTimeEvent(uint16 timeout, uint16 subroutine_id) {
+void AGOSEngine::addTimeEvent(int32 timeout, uint16 subroutine_id) {
 	TimeEvent *te = (TimeEvent *)malloc(sizeof(TimeEvent)), *first, *last = nullptr;
 	uint32 cur_time = getTime();
 
@@ -46,6 +46,12 @@ void AGOSEngine::addTimeEvent(uint16 timeout, uint16 subroutine_id) {
 		timeout /= 2;
 	}
 
+	if ((int32)(cur_time + timeout - _gameStoppedClock) < 0) {
+		// This basically fixes a signed/unsigned bug. See comment in AGOSEngine_Elvira2::loadGame().
+		warning("AGOSEngine::addTimeEvent(): Invalid timer encountered (probably from an older savegame) - applying workaround");
+		timeout = 0;
+	}
+
 	te->time = cur_time + timeout - _gameStoppedClock;
 	if (getGameType() == GType_FF && _clockStopped)
 		te->time -= (getTime() - _clockStopped);
diff --git a/engines/agos/saveload.cpp b/engines/agos/saveload.cpp
index d84364de10e..c0e746cb346 100644
--- a/engines/agos/saveload.cpp
+++ b/engines/agos/saveload.cpp
@@ -1087,7 +1087,8 @@ bool AGOSEngine::loadGame(const Common::String &filename, bool restartMode) {
 	// add all timers
 	killAllTimers();
 	for (num = f->readUint32BE(); num; num--) {
-		uint32 timeout = f->readUint32BE();
+		// See comment below inAGOSEngine_Elvira2::loadGame(): The timers are just as broken for Elvira as for the other games.
+		int32 timeout = (int16)f->readSint32BE();
 		uint16 subroutine_id = f->readUint16BE();
 		addTimeEvent(timeout, subroutine_id);
 	}
@@ -1272,7 +1273,15 @@ bool AGOSEngine_Elvira2::loadGame(const Common::String &filename, bool restartMo
 	// add all timers
 	killAllTimers();
 	for (num = f->readUint32BE(); num; num--) {
-		uint32 timeout = f->readUint32BE();
+		// WORKAROUND for older (corrupt) savegames. Games with short timer intervals may write negative timeouts into the save files. The
+		// original interpreter does that, too. I have checked it in the DOSBox debugger. We didn't handle this well, treating the negative
+		// values as very large positive values. This effectively disabled the timers. In most cases this seems to have gone unnoticed, but
+		// it also caused bug #14886 ("Waxworks crashing at Egypt Level 3, corrupting save file"). Waxworks runs a timer every 10 seconds
+		// that cleans up the items chain and failure to do so causes that bug. The design of the timers in the original interpreter is poor,
+		// but at least it somehow survives. Now, unfortunately, we don't have savegame versioning in this engine, so I can't simply limit
+		// a fix to old savegames. However, it is so highly unlikely that a valid timer would exceed 32767 seconds (= 9 hours) that I
+		// consider this safe.
+		int32 timeout = (int16)f->readSint32BE();
 		uint16 subroutine_id = f->readUint16BE();
 		addTimeEvent(timeout, subroutine_id);
 	}