[Scummvm-cvs-logs] SF.net SVN: scummvm:[53572] scummvm/trunk/engines/scumm

[fingolfin at users.sourceforge.net]

Revision: 53572
          http://scummvm.svn.sourceforge.net/scummvm/?rev=53572&view=rev
Author:   fingolfin
Date:     2010-10-18 18:55:24 +0000 (Mon, 18 Oct 2010)

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

Modified Paths:
--------------
    scummvm/trunk/engines/scumm/resource.cpp
    scummvm/trunk/engines/scumm/script.cpp
    scummvm/trunk/engines/scumm/scumm.h

Modified: scummvm/trunk/engines/scumm/resource.cpp
===================================================================
--- scummvm/trunk/engines/scumm/resource.cpp	2010-10-18 18:43:13 UTC (rev 53571)
+++ scummvm/trunk/engines/scumm/resource.cpp	2010-10-18 18:55:24 UTC (rev 53572)
@@ -1012,7 +1012,7 @@
 
 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 @@
 	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);
 	}
 }
 

Modified: scummvm/trunk/engines/scumm/script.cpp
===================================================================
--- scummvm/trunk/engines/scumm/script.cpp	2010-10-18 18:43:13 UTC (rev 53571)
+++ scummvm/trunk/engines/scumm/script.cpp	2010-10-18 18:55:24 UTC (rev 53572)
@@ -440,7 +440,6 @@
 	}
 }
 
-
 void ScummEngine::resetScriptPointer() {
 	if (_currentScript == 0xFF)
 		return;
@@ -1234,22 +1233,26 @@
 
 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) {

Modified: scummvm/trunk/engines/scumm/scumm.h
===================================================================
--- scummvm/trunk/engines/scumm/scumm.h	2010-10-18 18:43:13 UTC (rev 53571)
+++ scummvm/trunk/engines/scumm/scumm.h	2010-10-18 18:55:24 UTC (rev 53572)
@@ -776,7 +776,7 @@
 	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 */


This was sent by the SourceForge.net collaborative development platform, the world's largest Open Source development site.