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.

2 files changed, 17 insertions, 24 deletions

diff --git a/engines/sci/engine/script.cpp b/engines/sci/engine/script.cpp
index a8cee1f393..520275c55c 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 52b58eec2e..01f3e788b7 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.