From 3bc00e6633ad9d037a284ac33ebf442a478b59b0 Mon Sep 17 00:00:00 2001
From: Colin Snover
Date: Sat, 15 Jul 2017 16:34:39 -0500
Subject: 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.
---
 engines/sci/engine/object.cpp   |  8 ++++++++
 engines/sci/engine/savegame.cpp |  7 ++++---
 engines/sci/engine/script.cpp   | 15 +++++++--------
 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/engines/sci/engine/object.cpp b/engines/sci/engine/object.cpp
index d43cf509e3..8873812c88 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 995d7319c9..4ff27ba6d8 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 47acad2858..b3857f311c 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
-- 
cgit v1.2.3