From 6175c2bb190c1ef8280852a5ac01752666ee0de9 Mon Sep 17 00:00:00 2001
From: Max Horn <max@quendi.de>
Date: Mon, 18 Oct 2010 18:55:24 +0000
Subject: [PATCH] SCUMM: Fix potential bug in ScummEngine::resStrLen.

In particular, it might happen that ScummEngine::resStrLen is called
while the _scriptPointer is stale. In that case, it would be working
with the stale pointer. If the code calling it then uses fetchScript*()
methods to read the string whose length was just computed, then it would
read potentially *different* data (e.g. copyScriptString or
loadPtrToResource could have been affected).

I am not sure if this actually could have caused bugs somewhere; it might
even be provable that a script relocation cannot happen in all places
that invoke resStrLen. But for now it's much easier to make the code
safe than to verify that theory ;).

Also simplified some related code.

svn-id: r53572
---
 engines/scumm/resource.cpp | 13 +++++++------
 engines/scumm/script.cpp   | 25 ++++++++++++++-----------
 engines/scumm/scumm.h      |  2 +-
 3 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/engines/scumm/resource.cpp b/engines/scumm/resource.cpp
index d9a5e3414c3..5aae59d987d 100644
--- a/engines/scumm/resource.cpp
+++ b/engines/scumm/resource.cpp
@@ -1012,7 +1012,7 @@ void ResourceManager::freeResources() {
 
 void ScummEngine::loadPtrToResource(int type, int resindex, const byte *source) {
 	byte *alloced;
-	int i, len;
+	int len;
 
 	_res->nukeResource(type, resindex);
 
@@ -1024,12 +1024,13 @@ void ScummEngine::loadPtrToResource(int type, int resindex, const byte *source)
 	alloced = _res->createResource(type, resindex, len);
 
 	if (!source) {
-		alloced[0] = fetchScriptByte();
-		for (i = 1; i < len; i++)
-			alloced[i] = *_scriptPointer++;
+		// Need to refresh the script pointer, since createResource may
+		// have caused the script resource to expire.
+		refreshScriptPointer();
+		memcpy(alloced, _scriptPointer, len);
+		_scriptPointer += len;
 	} else {
-		for (i = 0; i < len; i++)
-			alloced[i] = source[i];
+		memcpy(alloced, source, len);
 	}
 }
 
diff --git a/engines/scumm/script.cpp b/engines/scumm/script.cpp
index 3c5dd9df6fb..b6058d4d9a3 100644
--- a/engines/scumm/script.cpp
+++ b/engines/scumm/script.cpp
@@ -440,7 +440,6 @@ void ScummEngine::getScriptBaseAddress() {
 	}
 }
 
-
 void ScummEngine::resetScriptPointer() {
 	if (_currentScript == 0xFF)
 		return;
@@ -1234,22 +1233,26 @@ bool ScummEngine::isRoomScriptRunning(int script) const {
 
 void ScummEngine::copyScriptString(byte *dst) {
 	int len = resStrLen(_scriptPointer) + 1;
-	while (len--)
-		*dst++ = fetchScriptByte();
+	memcpy(dst, _scriptPointer, len);
+	_scriptPointer += len;
+	dst += len;
 	*dst = 0;
 }
 
-//
-// Given a pointer to a Scumm string, this function returns the total byte length
-// of the string data in that resource. To do so it has to understand certain
-// special characters embedded into the string. The reason for this function is that
-// sometimes this embedded data contains zero bytes, thus we can't just use strlen.
-//
-int ScummEngine::resStrLen(const byte *src) const {
+/**
+ * Given a pointer to a Scumm string, this function returns the total
+ * byte length of the string data in that resource. To do so it has to
+ * understand certain special characters embedded into the string. The
+ * reason for this function is that sometimes this embedded data
+ * contains zero bytes, thus we can't just use strlen.
+ */
+int ScummEngine::resStrLen(const byte *src) {
 	int num = 0;
 	byte chr;
-	if (src == NULL)
+	if (src == NULL) {
+		refreshScriptPointer();
 		src = _scriptPointer;
+	}
 	while ((chr = *src++) != 0) {
 		num++;
 		if (_game.heversion <= 71 && chr == 0xFF) {
diff --git a/engines/scumm/scumm.h b/engines/scumm/scumm.h
index dc881ffa776..90b9240579c 100644
--- a/engines/scumm/scumm.h
+++ b/engines/scumm/scumm.h
@@ -776,7 +776,7 @@ protected:
 	void endOverride();
 
 	void copyScriptString(byte *dst);
-	int resStrLen(const byte *src) const;
+	int resStrLen(const byte *src);
 	void doSentence(int c, int b, int a);
 
 	/* Should be in Resource class */