From f6e4312665614d7b5b626f997194fbbcb00ddbb0 Mon Sep 17 00:00:00 2001
From: Filippos Karapetis <md5@scummvm.org>
Date: Mon, 2 Jul 2012 12:48:02 +0300
Subject: [PATCH] SCI: Add a hack for a bug in the script handling code

When resetting the segment manager, sometimes the locals block for a
script is placed in a segment smaller than the script itself. This
shouldn't be happening, but it isn't fatal, however it should be resolved
in a proper manner
---
 engines/sci/engine/seg_manager.cpp | 41 ++++++++++++++++--------------
 engines/sci/engine/seg_manager.h   |  8 ------
 2 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/engines/sci/engine/seg_manager.cpp b/engines/sci/engine/seg_manager.cpp
index a6c145979f69..d425e170cea7 100644
--- a/engines/sci/engine/seg_manager.cpp
+++ b/engines/sci/engine/seg_manager.cpp
@@ -143,16 +143,34 @@ Script *SegManager::allocateScript(int script_nr, SegmentId *segid) {
 }
 
 void SegManager::deallocate(SegmentId seg) {
-	if (!check(seg))
-		error("SegManager::deallocate(): invalid segment ID");
+	if (seg < 1 || (uint)seg >= _heap.size())
+		error("Attempt to deallocate an invalid segment ID");
 
 	SegmentObj *mobj = _heap[seg];
+	if (!mobj)
+		error("Attempt to deallocate an already freed segment");
 
 	if (mobj->getType() == SEG_TYPE_SCRIPT) {
 		Script *scr = (Script *)mobj;
 		_scriptSegMap.erase(scr->getScriptNumber());
-		if (scr->getLocalsSegment())
-			deallocate(scr->getLocalsSegment());
+		if (scr->getLocalsSegment()) {
+			// HACK: Check if the locals segment has already been deallocated.
+			// This happens sometimes in SQ4CD when resetting the segment
+			// manager: the locals for script 808 are somehow stored in a
+			// smaller segment than the script itself, so by the time the script
+			// is about to be freed, the locals block has already been freed.
+			// This isn't fatal, but it shouldn't be happening at all.
+			// FIXME: Check why this happens. Perhaps there's a bug in the
+			// script handling code?
+			if (!_heap[scr->getLocalsSegment()]) {
+				warning("SegManager::deallocate(): The locals block of script "
+						"%d has already been deallocated. Script segment: %d, "
+						"locals segment: %d", scr->getScriptNumber(), seg,
+						scr->getLocalsSegment());
+			} else {
+				deallocate(scr->getLocalsSegment());
+			}
+		}
 	}
 
 	delete mobj;
@@ -307,21 +325,6 @@ reg_t SegManager::findObjectByName(const Common::String &name, int index) {
 	return result[index];
 }
 
-// validate the seg
-// return:
-//	false - invalid seg
-//	true  - valid seg
-bool SegManager::check(SegmentId seg) {
-	if (seg < 1 || (uint)seg >= _heap.size()) {
-		return false;
-	}
-	if (!_heap[seg]) {
-		warning("SegManager: seg %x is removed from memory, but not removed from hash_map", seg);
-		return false;
-	}
-	return true;
-}
-
 // return the seg if script_id is valid and in the map, else 0
 SegmentId SegManager::getScriptSegment(int script_id) const {
 	return _scriptSegMap.getVal(script_id, 0);
diff --git a/engines/sci/engine/seg_manager.h b/engines/sci/engine/seg_manager.h
index 356a1b04a7d6..074d3f6b0a66 100644
--- a/engines/sci/engine/seg_manager.h
+++ b/engines/sci/engine/seg_manager.h
@@ -471,14 +471,6 @@ class SegManager : public Common::Serializable {
 	void createClassTable();
 
 	SegmentId findFreeSegment() const;
-
-	/**
-	 * Check segment validity
-	 * @param[in] seg	The segment to validate
-	 * @return 			false if 'seg' is an invalid segment, true if
-	 * 					'seg' is a valid segment
-	 */
-	bool check(SegmentId seg);
 };
 
 } // End of namespace Sci