SCI32: Fix invalid memory access after BitmapTable is extended

When new bitmaps are added and the underlying Common::Array needs
to move to expand, this invalidates all pointers to bitmaps, which
makes it basically impossible to use the bitmap segment since you
never know if a reference is going to be invalidated due to an
array move.

To solve this, BitmapTable is changed to hold pointers to
SciBitmaps that are allocated separately on the heap instead, so
when those bitmaps are looked up, the resulting pointers are valid
for the lifetime of the bitmap, instead of the lifetime of the
Common::Array used internally by BitmapTable.

5 files changed, 77 insertions, 30 deletions

diff --git a/engines/sci/engine/gc.cpp b/engines/sci/engine/gc.cpp
index ad4fd19405..6c1713bed9 100644
--- a/engines/sci/engine/gc.cpp
+++ b/engines/sci/engine/gc.cpp
@@ -160,7 +160,7 @@ AddrSet *findAllActiveReferences(EngineState *s) {
 				BitmapTable *bt = static_cast<BitmapTable *>(heap[i]);
 
 				for (uint j = 0; j < bt->_table.size(); j++) {
-					if (!bt->_table[j].data.getShouldGC()) {
+					if (bt->_table[j].data && bt->_table[j].data->getShouldGC() == false) {
 						wm.push(make_reg(i, j));
 					}
 				}
diff --git a/engines/sci/engine/savegame.cpp b/engines/sci/engine/savegame.cpp
index f49853de2f..62f9c5aada 100644
--- a/engines/sci/engine/savegame.cpp
+++ b/engines/sci/engine/savegame.cpp
@@ -159,7 +159,7 @@ void syncWithSerializer(Common::Serializer &s, SciString &obj) {
 	}
 }
 
-void syncWithSerializer(Common::Serializer &s, SciBitmap &obj) {
+void syncWithSerializer(Common::Serializer &s, SciBitmap *obj) {
 	debug("TODO: Sync bitmap");
 
 //	if (s.isSaving()) {
diff --git a/engines/sci/engine/seg_manager.cpp b/engines/sci/engine/seg_manager.cpp
index 67da4c847d..608452983a 100644
--- a/engines/sci/engine/seg_manager.cpp
+++ b/engines/sci/engine/seg_manager.cpp
@@ -959,7 +959,7 @@ 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;
@@ -977,21 +977,20 @@ SciBitmap *SegManager::lookupBitmap(const reg_t addr) {
 	BitmapTable &bitmapTable = *(BitmapTable *)_heap[addr.getSegment()];
 
 	if (!bitmapTable.isValidEntry(addr.getOffset()))
-		error("Attempt to use non-bitmap %04x:%04x as bitmap", PRINT_REG(addr));
+		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) {
 	if (_heap[addr.getSegment()]->getType() != SEG_TYPE_BITMAP)
-		error("Attempt to use non-bitmap %04x:%04x as bitmap", PRINT_REG(addr));
+		error("Attempt to free non-bitmap %04x:%04x as bitmap", PRINT_REG(addr));
 
 	BitmapTable &bitmapTable = *(BitmapTable *)_heap[addr.getSegment()];
 
 	if (!bitmapTable.isValidEntry(addr.getOffset()))
-		error("Attempt to use non-bitmap %04x:%04x as bitmap", PRINT_REG(addr));
+		error("Attempt to free invalid entry %04x:%04x as bitmap", PRINT_REG(addr));
 
-	bitmapTable.at(addr.getOffset()).destroy();
 	bitmapTable.freeEntry(addr.getOffset());
 }
 
diff --git a/engines/sci/engine/segment.cpp b/engines/sci/engine/segment.cpp
index 7f690cb7c4..7943946ee4 100644
--- a/engines/sci/engine/segment.cpp
+++ b/engines/sci/engine/segment.cpp
@@ -313,17 +313,6 @@ SegmentRef StringTable::dereference(reg_t pointer) {
 	return ret;
 }
 
-#pragma mark -
-#pragma mark Bitmaps
-
-SegmentRef BitmapTable::dereference(reg_t pointer) {
-	SegmentRef ret;
-	ret.isRaw = true;
-	ret.maxSize = at(pointer.getOffset()).getRawSize();
-	ret.raw = at(pointer.getOffset()).getRawData();
-	return ret;
-}
-
 #endif
 
 } // End of namespace Sci
diff --git a/engines/sci/engine/segment.h b/engines/sci/engine/segment.h
index c25ede1d6a..8e7cf9ef06 100644
--- a/engines/sci/engine/segment.h
+++ b/engines/sci/engine/segment.h
@@ -535,6 +535,8 @@ inline void set##property(uint##size value) {\
 	WRITE_SCI11ENDIAN_UINT##size(_data + (offset), (value));\
 }
 
+class BitmapTable;
+
 /**
  * A convenience class for creating and modifying in-memory
  * bitmaps.
@@ -583,6 +585,39 @@ public:
 
 	inline SciBitmap() : _data(nullptr), _dataSize(0), _gc(true) {}
 
+	inline SciBitmap(const SciBitmap &other) {
+		_dataSize = other._dataSize;
+		_data = (byte *)malloc(other._dataSize);
+		memcpy(_data, other._data, other._dataSize);
+		if (_dataSize) {
+			_buffer = Buffer(getWidth(), getHeight(), getPixels());
+		}
+		_gc = other._gc;
+	}
+
+	inline ~SciBitmap() {
+		free(_data);
+		_data = nullptr;
+		_dataSize = 0;
+	}
+
+	inline SciBitmap &operator=(const SciBitmap &other) {
+		if (this == &other) {
+			return *this;
+		}
+
+		free(_data);
+		_dataSize = other._dataSize;
+		_data = (byte *)malloc(other._dataSize);
+		memcpy(_data, other._data, _dataSize);
+		if (_dataSize) {
+			_buffer = Buffer(getWidth(), getHeight(), getPixels());
+		}
+		_gc = other._gc;
+
+		return *this;
+	}
+
 	/**
 	 * Allocates and initialises a new bitmap.
 	 */
@@ -613,12 +648,6 @@ public:
 		_buffer = Buffer(getWidth(), getHeight(), getPixels());
 	}
 
-	inline void destroy() {
-		free(_data);
-		_data = nullptr;
-		_dataSize = 0;
-	}
-
 	inline int getRawSize() const {
 		return _dataSize;
 	}
@@ -746,16 +775,46 @@ public:
 	}
 };
 
-struct BitmapTable : public SegmentObjTable<SciBitmap> {
-	BitmapTable() : SegmentObjTable<SciBitmap>(SEG_TYPE_BITMAP) {}
+struct BitmapTable : public SegmentObjTable<SciBitmap *> {
+	BitmapTable() : SegmentObjTable<SciBitmap *>(SEG_TYPE_BITMAP) {}
 
-	virtual void freeAtAddress(SegManager *segMan, reg_t sub_addr) {
-		at(sub_addr.getOffset()).destroy();
+	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());
 	}
 
+	SegmentRef dereference(reg_t pointer) {
+		SegmentRef ret;
+		ret.isRaw = true;
+		ret.maxSize = at(pointer.getOffset())->getRawSize();
+		ret.raw = at(pointer.getOffset())->getRawData();
+		return ret;
+	}
+
 	void saveLoadWithSerializer(Common::Serializer &ser);
-	SegmentRef dereference(reg_t pointer);
 };
 
 #endif