[Scummvm-git-logs] scummvm master -> a620b6c354a7eee9e521d7e94adc133b4d58b0a1

[csnover]

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:
a620b6c354 SCI: Fix handling of buggy SCI0 export tables


Commit: a620b6c354a7eee9e521d7e94adc133b4d58b0a1
    https://github.com/scummvm/scummvm/commit/a620b6c354a7eee9e521d7e94adc133b4d58b0a1
Author: Colin Snover (github.com at zetafleet.com)
Date: 2017-04-18T11:47:18-05:00

Commit Message:
SCI: Fix handling of buggy SCI0 export tables

The previous fix for this bug was incorrect; it only happened
to work because of another bug: the number of exports was being
read incorrectly (reading the byte size of the export block, not
the number of exports), so the validation check for the export
number always passed. Then, the "small" offsets that were seen
were actually either invalid reads into the header of the next
block in the script (KQ4), or reads into the bad first export
table which contained an unfilled offset (Camelot).

Once the incorrect number of exports was fixed, the previous "fix"
broke in KQ4 because the export number validation started to work
correctly and the first export table does not have enough entries
(needs 2, has 1).

This patch fixes the bug by using the last export table in SCI0
scripts instead of the first export table. (This does not affect
most scripts, since only the buggy scripts have more than one
export table.)

Fixes Trac#9731.

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


diff --git a/engines/sci/engine/script.cpp b/engines/sci/engine/script.cpp
index a8cee1f..520275c 100644
--- a/engines/sci/engine/script.cpp
+++ b/engines/sci/engine/script.cpp
@@ -162,7 +162,11 @@ void Script::load(int script_nr, ResourceManager *resMan, ScriptPatcher *scriptP
 	scriptPatcher->processScript(_nr, outBuffer);
 
 	if (getSciVersion() <= SCI_VERSION_1_LATE) {
-		SciSpan<const uint16> exportTable = findBlockSCI0(SCI_OBJ_EXPORTS).subspan<const uint16>(0);
+		// Some buggy game scripts contain two export tables (e.g. script 912
+		// in Camelot and script 306 in KQ4); in these scripts, the first table
+		// is broken, so we ignore it and use the last one instead
+		// Fixes bugs #3039785 and #3037595.
+		SciSpan<const uint16> exportTable = findBlockSCI0(SCI_OBJ_EXPORTS, true).subspan<const uint16>(0);
 		if (exportTable) {
 			// The export table is after the block header (4 bytes / 2 uint16s)
 			// and the number of exports (2 bytes / 1 uint16).
@@ -818,21 +822,6 @@ uint32 Script::validateExportFunc(int pubfunct, bool relocSci3) {
 			offset = relocateOffsetSci3(pubfunct * 2 + 22);
 	}
 
-	// Check if the offset found points to a second export table (e.g. script 912
-	// in Camelot and script 306 in KQ4). Such offsets are usually small (i.e. < 10),
-	// thus easily distinguished from actual code offsets.
-	// This only makes sense for SCI0-SCI1, as the export table in SCI1.1+ games
-	// is located at a specific address, thus findBlockSCI0() won't work.
-	// Fixes bugs #3039785 and #3037595.
-	if (offset < 10 && getSciVersion() <= SCI_VERSION_1_LATE) {
-		const SciSpan<const uint16> secondExportTable = findBlockSCI0(SCI_OBJ_EXPORTS, 0).subspan<const uint16>(0);
-
-		if (secondExportTable) {
-			// 3 skips header plus 2 bytes (secondExportTable is a uint16 pointer)
-			offset = secondExportTable.getUint16SEAt(3 + pubfunct);
-		}
-	}
-
 	// TODO: Check if this should be done for SCI1.1 games as well
 	if (getSciVersion() >= SCI_VERSION_2 && offset == 0) {
 		offset = _codeOffset;
@@ -844,11 +833,12 @@ uint32 Script::validateExportFunc(int pubfunct, bool relocSci3) {
 	return offset;
 }
 
-SciSpan<const byte> Script::findBlockSCI0(ScriptObjectTypes type, int startBlockIndex) {
-	SciSpan<const byte> buf = *_buf;
+SciSpan<const byte> Script::findBlockSCI0(ScriptObjectTypes type, bool findLastBlock) {
+	SciSpan<const byte> foundBlock;
+
 	bool oldScriptHeader = (getSciVersion() == SCI_VERSION_0_EARLY);
-	int blockIndex = 0;
 
+	SciSpan<const byte> buf = *_buf;
 	if (oldScriptHeader)
 		buf += 2;
 
@@ -862,15 +852,18 @@ SciSpan<const byte> Script::findBlockSCI0(ScriptObjectTypes type, int startBlock
 		const int blockSize = buf.getUint16LEAt(2);
 		assert(blockSize > 0);
 
-		if (blockType == type && blockIndex > startBlockIndex) {
-			return buf.subspan(0, blockSize, Common::String::format("%s, %s block", _buf->name().c_str(), sciObjectTypeNames[type]));
+		if (blockType == type) {
+			foundBlock = buf.subspan(0, blockSize, Common::String::format("%s, %s block", _buf->name().c_str(), sciObjectTypeNames[type]));;
+
+			if (!findLastBlock) {
+				break;
+			}
 		}
 
 		buf += blockSize;
-		blockIndex++;
 	}
 
-	return SciSpan<const byte>();
+	return foundBlock;
 }
 
 // memory operations
diff --git a/engines/sci/engine/script.h b/engines/sci/engine/script.h
index 52b58ee..01f3e78 100644
--- a/engines/sci/engine/script.h
+++ b/engines/sci/engine/script.h
@@ -246,7 +246,7 @@ public:
 	 * Finds the pointer where a block of a specific type starts from,
 	 * in SCI0 - SCI1 games
 	 */
-	SciSpan<const byte> findBlockSCI0(ScriptObjectTypes type, int startBlockIndex = -1);
+	SciSpan<const byte> findBlockSCI0(ScriptObjectTypes type, bool findLastBlock = false);
 
 	/**
 	 * Syncs the string heap of a script. Used when saving/loading.