SCI32: Add validity checks to kList iteration methods

In GK2, restoring a save game causes the segment manager to reset
in the middle of a kListFirstTrue call, which invalidates all
pointers and reg_ts to stored data. This means that when
kListFirstTrue tries to decrement the list recursion counter at
the end of iteration, it is writing to freed memory, potentially
resulting in heap corruption.

SCI3 added checks to prevent this from happening, but these checks
seem like they should have also been applied to some SCI2.1 games
as well (like GK2).

Since there should be no negative side-effect to this check, it
is applied universally to all SCI32 games.

3 files changed, 24 insertions, 15 deletions

diff --git a/engines/sci/engine/kgraphics32.cpp b/engines/sci/engine/kgraphics32.cpp
index 880ea0f559..a3f7e4fbd3 100644
--- a/engines/sci/engine/kgraphics32.cpp
+++ b/engines/sci/engine/kgraphics32.cpp
@@ -656,16 +656,9 @@ reg_t kBitmapCreate(EngineState *s, int argc, reg_t *argv) {
 }
 
 reg_t kBitmapDestroy(EngineState *s, int argc, reg_t *argv) {
-	const reg_t &addr = argv[0];
-	const SegmentObj *const segment = s->_segMan->getSegmentObj(addr.getSegment());
-
-	if (segment != nullptr &&
-		segment->getType() == SEG_TYPE_BITMAP &&
-		segment->isValidOffset(addr.getOffset())) {
-
-		s->_segMan->freeBitmap(addr);
+	if (s->_segMan->isValidAddr(argv[0], SEG_TYPE_BITMAP)) {
+		s->_segMan->freeBitmap(argv[0]);
 	}
-
 	return s->r_acc;
 }
 
diff --git a/engines/sci/engine/klists.cpp b/engines/sci/engine/klists.cpp
index de5d7f1536..305d459ac1 100644
--- a/engines/sci/engine/klists.cpp
+++ b/engines/sci/engine/klists.cpp
@@ -586,7 +586,8 @@ reg_t kListIndexOf(EngineState *s, int argc, reg_t *argv) {
 }
 
 reg_t kListEachElementDo(EngineState *s, int argc, reg_t *argv) {
-	List *list = s->_segMan->lookupList(argv[0]);
+	const reg_t listReg = argv[0];
+	List *list = s->_segMan->lookupList(listReg);
 
 	Node *curNode = s->_segMan->lookupNode(list->first);
 	Selector slc = argv[1].toUint16();
@@ -627,13 +628,16 @@ reg_t kListEachElementDo(EngineState *s, int argc, reg_t *argv) {
 		curNode = s->_segMan->lookupNode(list->nextNodes[list->numRecursions]);
 	}
 
-	--list->numRecursions;
+	if (s->_segMan->isValidAddr(listReg, SEG_TYPE_LISTS)) {
+		--list->numRecursions;
+	}
 
 	return s->r_acc;
 }
 
 reg_t kListFirstTrue(EngineState *s, int argc, reg_t *argv) {
-	List *list = s->_segMan->lookupList(argv[0]);
+	const reg_t listReg = argv[0];
+	List *list = s->_segMan->lookupList(listReg);
 
 	Node *curNode = s->_segMan->lookupNode(list->first);
 	Selector slc = argv[1].toUint16();
@@ -678,13 +682,16 @@ reg_t kListFirstTrue(EngineState *s, int argc, reg_t *argv) {
 		curNode = s->_segMan->lookupNode(list->nextNodes[list->numRecursions]);
 	}
 
-	--list->numRecursions;
+	if (s->_segMan->isValidAddr(listReg, SEG_TYPE_LISTS)) {
+		--list->numRecursions;
+	}
 
 	return s->r_acc;
 }
 
 reg_t kListAllTrue(EngineState *s, int argc, reg_t *argv) {
-	List *list = s->_segMan->lookupList(argv[0]);
+	const reg_t listReg = argv[0];
+	List *list = s->_segMan->lookupList(listReg);
 
 	Node *curNode = s->_segMan->lookupNode(list->first);
 	reg_t curObject;
@@ -723,7 +730,9 @@ reg_t kListAllTrue(EngineState *s, int argc, reg_t *argv) {
 		curNode = s->_segMan->lookupNode(list->nextNodes[list->numRecursions]);
 	}
 
-	--list->numRecursions;
+	if (s->_segMan->isValidAddr(listReg, SEG_TYPE_LISTS)) {
+		--list->numRecursions;
+	}
 
 	return s->r_acc;
 }
diff --git a/engines/sci/engine/seg_manager.h b/engines/sci/engine/seg_manager.h
index e3ccb9ae5f..916b813eb5 100644
--- a/engines/sci/engine/seg_manager.h
+++ b/engines/sci/engine/seg_manager.h
@@ -432,6 +432,13 @@ public:
 	reg_t getParserPtr() const { return _parserPtr; }
 
 #ifdef ENABLE_SCI32
+	bool isValidAddr(reg_t reg, SegmentType expected) const {
+		SegmentObj *mobj = getSegmentObj(reg.getSegment());
+		return (mobj &&
+				mobj->getType() == expected &&
+				mobj->isValidOffset(reg.getOffset()));
+	}
+
 	SciArray *allocateArray(SciArrayType type, uint16 size, reg_t *addr);
 	SciArray *lookupArray(reg_t addr);
 	void freeArray(reg_t addr);