SCI: Fix pointer invalidation caused by array storage moves

When objects are added to a SegmentObjTable, it may cause the
internal storage for the table to expand and move to a new
region of memory. When this happens, all pointers to objects held
by a SegmentObjTable of the same type would be invalidated, due
to an implementation detail that should not be exposed. To prevent
this, objects are now allocated separately on the heap, so even if
the table's storage moves due to insertions, the objects owned by
the table will not, so references remain valid for the lifetime of
the object.

4 files changed, 53 insertions, 68 deletions

diff --git a/engines/sci/engine/savegame.cpp b/engines/sci/engine/savegame.cpp
index c4d53a2dc9..c5d2834e58 100644
--- a/engines/sci/engine/savegame.cpp
+++ b/engines/sci/engine/savegame.cpp
@@ -159,23 +159,6 @@ void syncWithSerializer(Common::Serializer &s, SciString &obj) {
 	}
 }
 
-void syncWithSerializer(Common::Serializer &s, SciBitmap *&obj) {
-	bool hasEntry;
-	if (s.isSaving()) {
-		hasEntry = obj != nullptr;
-	}
-	s.syncAsByte(hasEntry);
-
-	if (hasEntry) {
-		if (s.isLoading()) {
-			obj = new SciBitmap;
-		}
-
-		obj->saveLoadWithSerializer(s);
-	} else {
-		obj = nullptr;
-	}
-}
 #endif
 
 #pragma mark -
@@ -183,7 +166,7 @@ void syncWithSerializer(Common::Serializer &s, SciBitmap *&obj) {
 // By default, sync using syncWithSerializer, which in turn can easily be overloaded.
 template<typename T>
 struct DefaultSyncer : Common::BinaryFunction<Common::Serializer, T, void> {
-	void operator()(Common::Serializer &s, T &obj) const {
+	void operator()(Common::Serializer &s, T &obj, int) const {
 		syncWithSerializer(s, obj);
 	}
 };
@@ -191,10 +174,31 @@ struct DefaultSyncer : Common::BinaryFunction<Common::Serializer, T, void> {
 // Syncer for entries in a segment obj table
 template<typename T>
 struct SegmentObjTableEntrySyncer : Common::BinaryFunction<Common::Serializer, typename T::Entry &, void> {
-	void operator()(Common::Serializer &s, typename T::Entry &entry) const {
+	void operator()(Common::Serializer &s, typename T::Entry &entry, int index) const {
 		s.syncAsSint32LE(entry.next_free);
 
-		syncWithSerializer(s, entry.data);
+		bool hasData;
+		if (s.getVersion() >= 37) {
+			if (s.isSaving()) {
+				hasData = entry.data != nullptr;
+			}
+			s.syncAsByte(hasData);
+		} else {
+			hasData = (entry.next_free == index);
+		}
+
+		if (hasData) {
+			if (s.isLoading()) {
+				entry.data = new typename T::value_type;
+			}
+			syncWithSerializer(s, *entry.data);
+		} else if (s.isLoading()) {
+			if (s.getVersion() < 37) {
+				typename T::value_type dummy;
+				syncWithSerializer(s, dummy);
+			}
+			entry.data = nullptr;
+		}
 	}
 };
 
@@ -222,9 +226,8 @@ struct ArraySyncer : Common::BinaryFunction<Common::Serializer, T, void> {
 		if (s.isLoading())
 			arr.resize(len);
 
-		typename Common::Array<T>::iterator i;
-		for (i = arr.begin(); i != arr.end(); ++i) {
-			sync(s, *i);
+		for (int i = 0; i < len; ++i) {
+			sync(s, arr[i], i);
 		}
 	}
 };
diff --git a/engines/sci/engine/savegame.h b/engines/sci/engine/savegame.h
index c5c2bcef08..1bf66864e8 100644
--- a/engines/sci/engine/savegame.h
+++ b/engines/sci/engine/savegame.h
@@ -37,6 +37,7 @@ struct EngineState;
  *
  * Version - new/changed feature
  * =============================
+ *      37 - Segment entry data changed to pointers
  *      36 - SCI32 bitmap segment
  *      35 - SCI32 remap
  *      34 - SCI32 palettes, and store play time in ticks
@@ -61,7 +62,7 @@ struct EngineState;
  */
 
 enum {
-	CURRENT_SAVEGAME_VERSION = 36,
+	CURRENT_SAVEGAME_VERSION = 37,
 	MINIMUM_SAVEGAME_VERSION = 14
 };
 
diff --git a/engines/sci/engine/seg_manager.cpp b/engines/sci/engine/seg_manager.cpp
index 608452983a..5cf8d6162d 100644
--- a/engines/sci/engine/seg_manager.cpp
+++ b/engines/sci/engine/seg_manager.cpp
@@ -959,15 +959,11 @@ SciBitmap *SegManager::allocateBitmap(reg_t *addr, const int16 width, const int1
 	offset = table->allocEntry();
 
 	*addr = make_reg(_bitmapSegId, offset);
-	SciBitmap *bitmap = table->at(offset);
+	SciBitmap &bitmap = table->at(offset);
 
-	if (bitmap == nullptr) {
-		*addr = NULL_REG;
-	}
-
-	bitmap->create(width, height, skipColor, displaceX, displaceY, scaledWidth, scaledHeight, paletteSize, remap, gc);
+	bitmap.create(width, height, skipColor, displaceX, displaceY, scaledWidth, scaledHeight, paletteSize, remap, gc);
 
-	return bitmap;
+	return &bitmap;
 }
 
 SciBitmap *SegManager::lookupBitmap(const reg_t addr) {
@@ -979,7 +975,7 @@ SciBitmap *SegManager::lookupBitmap(const reg_t addr) {
 	if (!bitmapTable.isValidEntry(addr.getOffset()))
 		error("Attempt to use invalid entry %04x:%04x as bitmap", PRINT_REG(addr));
 
-	return (bitmapTable.at(addr.getOffset()));
+	return &(bitmapTable.at(addr.getOffset()));
 }
 
 void SegManager::freeBitmap(const reg_t addr) {
diff --git a/engines/sci/engine/segment.h b/engines/sci/engine/segment.h
index 97a6cb585f..add5f4c57c 100644
--- a/engines/sci/engine/segment.h
+++ b/engines/sci/engine/segment.h
@@ -227,7 +227,7 @@ template<typename T>
 struct SegmentObjTable : public SegmentObj {
 	typedef T value_type;
 	struct Entry {
-		T data;
+		T *data;
 		int next_free; /* Only used for free entries */
 	};
 	enum { HEAPENTRY_INVALID = -1 };
@@ -243,6 +243,14 @@ public:
 		initTable();
 	}
 
+	~SegmentObjTable() {
+		for (uint i = 0; i < _table.size(); i++) {
+			if (isValidEntry(i)) {
+				freeEntry(i);
+			}
+		}
+	}
+
 	void initTable() {
 		entries_used = 0;
 		first_free = HEAPENTRY_INVALID;
@@ -256,10 +264,13 @@ public:
 			first_free = _table[oldff].next_free;
 
 			_table[oldff].next_free = oldff;
+			assert(_table[oldff].data == nullptr);
+			_table[oldff].data = new T;
 			return oldff;
 		} else {
 			uint newIdx = _table.size();
 			_table.push_back(Entry());
+			_table.back().data = new T;
 			_table[newIdx].next_free = newIdx;	// Tag as 'valid'
 			return newIdx;
 		}
@@ -278,6 +289,8 @@ public:
 			::error("Table::freeEntry: Attempt to release invalid table index %d", idx);
 
 		_table[idx].next_free = first_free;
+		delete _table[idx].data;
+		_table[idx].data = nullptr;
 		first_free = idx;
 		entries_used--;
 	}
@@ -292,8 +305,8 @@ public:
 
 	uint size() const { return _table.size(); }
 
-	T &at(uint index) { return _table[index].data; }
-	const T &at(uint index) const { return _table[index].data; }
+	T &at(uint index) { return *_table[index].data; }
+	const T &at(uint index) const { return *_table[index].data; }
 
 	T &operator[](uint index) { return at(index); }
 	const T &operator[](uint index) const { return at(index); }
@@ -353,8 +366,8 @@ struct HunkTable : public SegmentObjTable<Hunk> {
 	}
 
 	virtual void freeEntry(int idx) {
-		SegmentObjTable<Hunk>::freeEntry(idx);
 		freeEntryContents(idx);
+		SegmentObjTable<Hunk>::freeEntry(idx);
 	}
 
 	virtual void freeAtAddress(SegManager *segMan, reg_t sub_addr) {
@@ -792,42 +805,14 @@ public:
 	virtual void saveLoadWithSerializer(Common::Serializer &ser);
 };
 
-struct BitmapTable : public SegmentObjTable<SciBitmap *> {
-	BitmapTable() : SegmentObjTable<SciBitmap *>(SEG_TYPE_BITMAP) {}
-
-	virtual ~BitmapTable() {
-		for (uint i = 0; i < _table.size(); i++) {
-			if (isValidEntry(i)) {
-				freeEntryContents(i);
-			}
-		}
-	}
-
-	int allocEntry() {
-		int offset = SegmentObjTable<SciBitmap *>::allocEntry();
-		at(offset) = new SciBitmap;
-		return offset;
-	}
-
-	void freeEntryContents(const int offset) {
-		delete at(offset);
-		at(offset) = nullptr;
-	}
-
-	virtual void freeEntry(const int offset) override {
-		SegmentObjTable<SciBitmap *>::freeEntry(offset);
-		freeEntryContents(offset);
-	}
-
-	virtual void freeAtAddress(SegManager *segMan, reg_t sub_addr) override {
-		freeEntry(sub_addr.getOffset());
-	}
+struct BitmapTable : public SegmentObjTable<SciBitmap> {
+	BitmapTable() : SegmentObjTable<SciBitmap>(SEG_TYPE_BITMAP) {}
 
 	SegmentRef dereference(reg_t pointer) {
 		SegmentRef ret;
 		ret.isRaw = true;
-		ret.maxSize = at(pointer.getOffset())->getRawSize();
-		ret.raw = at(pointer.getOffset())->getRawData();
+		ret.maxSize = at(pointer.getOffset()).getRawSize();
+		ret.raw = at(pointer.getOffset()).getRawData();
 		return ret;
 	}