[Scummvm-git-logs] scummvm master -> 3bc00e6633ad9d037a284ac33ebf442a478b59b0

csnover csnover at users.noreply.github.com
Sun Jul 16 03:37:46 CEST 2017


This automated email contains information about 2 new commits which have been
pushed to the 'scummvm' repo located at https://github.com/scummvm/scummvm .

Summary:
051366a48a SCI: Fix up Object::_baseMethod implementation
3bc00e6633 SCI: Stop double-initialization of SCI0/1 objects


Commit: 051366a48a30e68efcfe9ebb584a3774ca357e1e
    https://github.com/scummvm/scummvm/commit/051366a48a30e68efcfe9ebb584a3774ca357e1e
Author: Colin Snover (github.com at zetafleet.com)
Date: 2017-07-15T20:10:38-05:00

Commit Message:
SCI: Fix up Object::_baseMethod implementation

1. In SCI0/1, selectors and offsets in the method block are
   stored contiguously (all selectors, then all offsets), with a
   null separator between the two runs. All the later versions of
   SCI instead interleave selectors & offsets. Since these values
   are already being copied into a new array anyway, code for
   reading method selectors/offsets is now simplified by
   interleaving this data when it is written into _baseMethod
   for SCI0/1, so the same equation for retrieving method
   selectors/offsets can be used across all SCI versions.

2. In SCI1.1-2.1 branch, the method count was being copied into
   the first entry of the array, which meant that SCI1.1-2.1 had
   extra code for dealing with the fact that the first entry was
   not an entry. This has been fixed, and the extra code removed.

3. Data was being overread into _baseMethod in all games SCI0-2.1.
   (SCI0/1 had an extra magic value of 2, and SCI1.1-2.1 had an
   extra magic value of 3). Reviewing history, it's not clear why
   this happened, other than that it appears to have been
   introduced at 7b0760f1bc5c28abcede041a6e3930f84ff3d319. My best
   guess is that this was a confusion between byte count and record
   count, where the intent was to read an extra 2 bytes for the
   null separator in SCI0/1, but it actually read 2 records
   instead. (I do not have a guess on why SCI1.1 ended up with a
   3.) This overreading has been removed.

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


diff --git a/engines/sci/engine/object.cpp b/engines/sci/engine/object.cpp
index 5d85bf6..d43cf50 100644
--- a/engines/sci/engine/object.cpp
+++ b/engines/sci/engine/object.cpp
@@ -54,9 +54,29 @@ void Object::init(const Script &owner, reg_t obj_pos, bool initVariables) {
 			}
 		}
 
-		_methodCount = data.getUint16LEAt(header.getUint16LEAt(kOffsetHeaderFunctionArea) - 2);
-		for (uint i = 0; i < _methodCount * sizeof(uint16) + 2; ++i) {
-			_baseMethod.push_back(data.getUint16SEAt(header.getUint16LEAt(kOffsetHeaderFunctionArea) + i * sizeof(uint16)));
+		// method block structure:
+		// uint16 count;
+		// uint16 selectorNos[count];
+		// uint16 zero;
+		// uint16 codeOffsets[count];
+
+		const uint16 methodBlockOffset = header.getUint16LEAt(kOffsetHeaderFunctionArea) - 2;
+		_methodCount = data.getUint16LEAt(methodBlockOffset);
+		const uint32 methodBlockSize = _methodCount * 2 * sizeof(uint16) + /* zero-terminator after selector list */ sizeof(uint16);
+
+		SciSpan<const uint16> methodEntries = data.subspan<const uint16>(methodBlockOffset + /* count */ sizeof(uint16), methodBlockSize);
+
+		// If this happens, then there is either a corrupt script or this code
+		// misunderstands the structure of the SCI0/1 method block
+		if (methodEntries.getUint16SEAt(_methodCount) != 0) {
+			warning("Object %04x:%04x in script %u has a value (0x%04x) in its zero-terminator field", PRINT_REG(obj_pos), owner.getScriptNumber(), methodEntries.getUint16SEAt(_methodCount));
+		}
+
+		_baseMethod.reserve(_methodCount * 2);
+		for (uint i = 0; i < _methodCount; ++i) {
+			_baseMethod.push_back(methodEntries.getUint16SEAt(0));
+			_baseMethod.push_back(methodEntries.getUint16SEAt(_methodCount + /* zero-terminator */ 1));
+			++methodEntries;
 		}
 	} else if (getSciVersion() >= SCI_VERSION_1_1 && getSciVersion() <= SCI_VERSION_2_1_LATE) {
 		_variables.resize(data.getUint16SEAt(2));
@@ -72,9 +92,27 @@ void Object::init(const Script &owner, reg_t obj_pos, bool initVariables) {
 			}
 		}
 
-		_methodCount = buf.getUint16SEAt(data.getUint16SEAt(6));
-		for (uint i = 0; i < _methodCount * sizeof(uint16) + 3; ++i) {
-			_baseMethod.push_back(buf.getUint16SEAt(data.getUint16SEAt(6) + i * sizeof(uint16)));
+		// method block structure:
+		// uint16 count;
+		// struct {
+		//   uint16 selectorNo;
+		//   uint16 codeOffset;
+		// } entries[count];
+
+		const uint16 methodBlockOffset = data.getUint16SEAt(6);
+		_methodCount = buf.getUint16SEAt(methodBlockOffset);
+
+		// Each entry in _baseMethod is actually two values; the first field is
+		// a selector number, and the second field is an offset to the method's
+		// code in the script
+		const uint32 methodBlockSize = _methodCount * 2 * sizeof(uint16);
+		_baseMethod.reserve(_methodCount * 2);
+
+		SciSpan<const uint16> methodEntries = buf.subspan<const uint16>(methodBlockOffset + /* count */ sizeof(uint16), methodBlockSize);
+		for (uint i = 0; i < _methodCount; ++i) {
+			_baseMethod.push_back(methodEntries.getUint16SEAt(0));
+			_baseMethod.push_back(methodEntries.getUint16SEAt(1));
+			methodEntries += 2;
 		}
 #ifdef ENABLE_SCI32
 	} else if (getSciVersion() == SCI_VERSION_3) {
diff --git a/engines/sci/engine/object.h b/engines/sci/engine/object.h
index 8b597b4..ffc4ac2 100644
--- a/engines/sci/engine/object.h
+++ b/engines/sci/engine/object.h
@@ -230,22 +230,21 @@ public:
 
 	Selector getVarSelector(uint16 i) const { return _baseVars[i]; }
 
-	reg_t getFunction(uint16 i) const {
-		uint16 offset = (getSciVersion() < SCI_VERSION_1_1) ? _methodCount + 1 + i : i * 2 + 2;
-		if (getSciVersion() == SCI_VERSION_3)
-			offset--;
-
+	/**
+	 * @returns A pointer to the code for the method at the given index.
+	 */
+	reg_t getFunction(const uint16 index) const {
 		reg_t addr;
 		addr.setSegment(_pos.getSegment());
-		addr.setOffset(_baseMethod[offset]);
+		addr.setOffset(_baseMethod[index * 2 + 1]);
 		return addr;
 	}
 
-	Selector getFuncSelector(uint16 i) const {
-		uint16 offset = (getSciVersion() < SCI_VERSION_1_1) ? i : i * 2 + 1;
-		if (getSciVersion() == SCI_VERSION_3)
-			offset--;
-		return _baseMethod[offset];
+	/**
+	 * @returns The selector for the method at the given index.
+	 */
+	Selector getFuncSelector(const uint16 index) const {
+		return _baseMethod[index * 2];
 	}
 
 	/**
@@ -335,8 +334,8 @@ private:
 	Common::Array<uint16> _baseVars;
 
 	/**
-	 * A lookup table from a method index to its corresponding selector number.
-	 * In SCI3, the table contains selector + offset in pairs.
+	 * A lookup table from a method index to its corresponding selector number
+	 * or offset to code. The table contains selector + offset in pairs.
 	 */
 	Common::Array<uint32> _baseMethod;
 


Commit: 3bc00e6633ad9d037a284ac33ebf442a478b59b0
    https://github.com/scummvm/scummvm/commit/3bc00e6633ad9d037a284ac33ebf442a478b59b0
Author: Colin Snover (github.com at zetafleet.com)
Date: 2017-07-15T20:29:36-05:00

Commit Message:
SCI: Stop double-initialization of SCI0/1 objects

These objects should have been initialized only during the first
pass. Double-initialization does not cause any visible problem
problem during normal operation (mostly it just causes memory
waste by making Object::_baseVars/_baseMethod double up their
data), but could have silently allowed games to receive bogus data
for an out-of-bounds property or method index, instead of raising
an error.

Changed paths:
    engines/sci/engine/object.cpp
    engines/sci/engine/savegame.cpp
    engines/sci/engine/script.cpp


diff --git a/engines/sci/engine/object.cpp b/engines/sci/engine/object.cpp
index d43cf50..8873812 100644
--- a/engines/sci/engine/object.cpp
+++ b/engines/sci/engine/object.cpp
@@ -39,6 +39,14 @@ void Object::init(const Script &owner, reg_t obj_pos, bool initVariables) {
 	_baseObj = data;
 	_pos = obj_pos;
 
+	// Calling Object::init more than once will screw up _baseVars/_baseMethod
+	// by duplicating data. This could be turned into a soft error by warning
+	// instead and clearing arrays, but there does not currently seem to be any
+	// reason for an object to be initialized multiple times
+	if (_baseVars.size() || _baseMethod.size()) {
+		error("Attempt to reinitialize already-initialized object %04x:%04x in script %u", PRINT_REG(obj_pos), owner.getScriptNumber());
+	}
+
 	if (getSciVersion() <= SCI_VERSION_1_LATE) {
 		const SciSpan<const byte> header = buf.subspan(obj_pos.getOffset() - kOffsetHeaderSize);
 		_variables.resize(header.getUint16LEAt(kOffsetHeaderSelectorCounter));
diff --git a/engines/sci/engine/savegame.cpp b/engines/sci/engine/savegame.cpp
index 995d731..4ff27ba 100644
--- a/engines/sci/engine/savegame.cpp
+++ b/engines/sci/engine/savegame.cpp
@@ -280,9 +280,10 @@ void SegManager::saveLoadWithSerializer(Common::Serializer &s) {
 				ObjMap &objects = scr->getObjectMap();
 				for (ObjMap::iterator it = objects.begin(); it != objects.end(); ++it) {
 					reg_t addr = it->_value.getPos();
-					Object *obj = scr->scriptObjInit(addr, false);
-
-					if (pass == 2) {
+					if (pass == 1) {
+						scr->scriptObjInit(addr, false);
+					} else {
+						Object *obj = scr->getObject(addr.getOffset());
 						// When a game disposes a script with kDisposeScript,
 						// the script is marked as deleted and its lockers are
 						// set to 0, which makes the GC stop using the script
diff --git a/engines/sci/engine/script.cpp b/engines/sci/engine/script.cpp
index 47acad2..b3857f3 100644
--- a/engines/sci/engine/script.cpp
+++ b/engines/sci/engine/script.cpp
@@ -649,9 +649,6 @@ const Object *Script::getObject(uint32 offset) const {
 }
 
 Object *Script::scriptObjInit(reg_t obj_pos, bool fullObjectInit) {
-	if (getSciVersion() < SCI_VERSION_1_1 && fullObjectInit)
-		obj_pos.incOffset(8);	// magic offset (SCRIPT_OBJECT_MAGIC_OFFSET)
-
 	if (obj_pos.getOffset() >= _buf->size())
 		error("Attempt to initialize object beyond end of script %d (%u >= %u)", _nr, obj_pos.getOffset(), _buf->size());
 
@@ -1088,11 +1085,13 @@ void Script::initializeObjectsSci0(SegManager *segMan, SegmentId segmentId) {
 			case SCI_OBJ_OBJECT:
 			case SCI_OBJ_CLASS:
 				{
-					reg_t addr = make_reg(segmentId, seeker - *_buf + 4);
-					Object *obj = scriptObjInit(addr);
-					obj->initSpecies(segMan, addr);
-
-					if (pass == 2) {
+					reg_t addr = make_reg(segmentId, seeker - *_buf + 4 - SCRIPT_OBJECT_MAGIC_OFFSET);
+					Object *obj;
+					if (pass == 1) {
+						obj = scriptObjInit(addr);
+						obj->initSpecies(segMan, addr);
+					} else {
+						obj = getObject(addr.getOffset());
 						if (!obj->initBaseObject(segMan, addr)) {
 							if ((_nr == 202 || _nr == 764) && g_sci->getGameId() == GID_KQ5) {
 								// WORKAROUND: Script 202 of KQ5 French and German





More information about the Scummvm-git-logs mailing list