From ceefad66349ffbd6a935af3d8ebfcf8d02772618 Mon Sep 17 00:00:00 2001
From: Max Horn
Date: Mon, 21 Aug 2006 13:02:47 +0000
Subject: Added workaround (and warnings to find corner cases) for bug #1535358

svn-id: r23738
---
 engines/scumm/object.cpp | 44 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/engines/scumm/object.cpp b/engines/scumm/object.cpp
index 731e7b3286..85c4d46247 100644
--- a/engines/scumm/object.cpp
+++ b/engines/scumm/object.cpp
@@ -929,8 +929,7 @@ void ScummEngine::clearOwnerOf(int obj) {
 	stopObjectScript(obj);
 
 	if (getOwner(obj) == OF_OWNER_ROOM) {
-		i = 0;
-		do {
+		for (i = 0; i < _numLocalObjects; i++)  {
 			if (_objs[i].obj_nr == obj) {
 				if (!_objs[i].fl_object_index)
 					return;
@@ -938,7 +937,7 @@ void ScummEngine::clearOwnerOf(int obj) {
 				_objs[i].obj_nr = 0;
 				_objs[i].fl_object_index = 0;
 			}
-		} while (++i < _numLocalObjects);
+		}
 		return;
 	}
 
@@ -1386,12 +1385,41 @@ void ScummEngine::setOwnerOf(int obj, int owner) {
 
 	if (owner == 0) {
 		clearOwnerOf(obj);
+		
+		// FIXME: See bug #1535358 and many others. Essentially, the following
+		// code, while matching disasm of various versions of the SCUMM engine,
+		// is total bullocks, and leads to odd crashes due to out-of-bounds
+		// array (read) access. Three "famous" crashes were caused by this:
+		//   Monkey Island 1: Using meat with flower
+		//   FOA: Using ribcage with another item
+		//   DOTT: Using stamp with contract
+		//
+		// The bad code:
+		//   if (ss->where == WIO_INVENTORY && _inventory[ss->number] == obj) {
+		// That check makes no sense at all: _inventory only contains 80 items,
+		// which are in the order the player picked up items. We can only 
+		// guess that the SCUMM coders meant to write
+		//   if (ss->where == WIO_INVENTORY && ss->number == obj) {
+		// which would ensure that an object script that nukes itself gets
+		// stopped. Alas, we can't just make that change, since it could
+		// lead to new regressions.
+		// Another fix would be to completely remove this check, which should
+		// not cause much problems, since it'll only succeed by pure chance.
+		// 
+		// For now we follow a more defensive route: We perform the check
+		// if ss->number is small enough.
+		
 		ss = &vm.slot[_currentScript];
-		if (ss->where == WIO_INVENTORY && _inventory[ss->number] == obj) {
-			putOwner(obj, 0);
-			runInventoryScript(arg);
-			stopObjectCode();
-			return;
+		if (ss->where == WIO_INVENTORY) {
+			if (ss->number < _numInventory && _inventory[ss->number] == obj) {
+				warning("Odd setOwnerOf case #1: Please report to Fingolfin where you encountered this");
+				putOwner(obj, 0);
+				runInventoryScript(arg);
+				stopObjectCode();
+				return;
+			}
+			if (ss->number == obj)
+				warning("Odd setOwnerOf case #2: Please report to Fingolfin where you encountered this");
 		}
 	}
 
-- 
cgit v1.2.3