SCI32: Add some bounds checking, const-correctness, and errors to CelObj

2 files changed, 65 insertions, 51 deletions

diff --git a/engines/sci/graphics/celobj32.cpp b/engines/sci/graphics/celobj32.cpp
index ebe845b567..c5b1133a5f 100644
--- a/engines/sci/graphics/celobj32.cpp
+++ b/engines/sci/graphics/celobj32.cpp
@@ -45,7 +45,7 @@ void CelScaler::activateScaleTables(const Ratio &scaleX, const Ratio &scaleY) {
 		}
 	}
 
-	int i = 1 - _activeIndex;
+	const int i = 1 - _activeIndex;
 	_activeIndex = i;
 	CelScalerTable &table = _scaleTables[i];
 
@@ -65,7 +65,7 @@ void CelScaler::activateScaleTables(const Ratio &scaleX, const Ratio &scaleY) {
 void CelScaler::buildLookupTable(int *table, const Ratio &ratio, const int size) {
 	int value = 0;
 	int remainder = 0;
-	int num = ratio.getNumerator();
+	const int num = ratio.getNumerator();
 	for (int i = 0; i < size; ++i) {
 		*table++ = value;
 		remainder += ratio.getDenominator();
@@ -204,7 +204,7 @@ struct SCALER_Scale {
 		if (g_sci->_gfxFrameout->getCurrentBuffer().scriptWidth == kLowResX) {
 			const int16 unscaledX = (scaledPosition.x / scaleX).toInt();
 			if (FLIP) {
-				int lastIndex = celObj._width - 1;
+				const int lastIndex = celObj._width - 1;
 				for (int16 x = targetRect.left; x < targetRect.right; ++x) {
 					_valuesX[x] = lastIndex - (table->valuesX[x] - unscaledX);
 				}
@@ -220,7 +220,7 @@ struct SCALER_Scale {
 			}
 		} else {
 			if (FLIP) {
-				int lastIndex = celObj._width - 1;
+				const int lastIndex = celObj._width - 1;
 				for (int16 x = 0; x < targetRect.width(); ++x) {
 					_valuesX[targetRect.left + x] = lastIndex - table->valuesX[x];
 				}
@@ -261,7 +261,7 @@ private:
 #ifndef NDEBUG
 	const int16 _sourceHeight;
 #endif
-	byte *_pixels;
+	const byte *_pixels;
 	const int16 _sourceWidth;
 
 public:
@@ -270,7 +270,7 @@ public:
 	_sourceHeight(celObj._height),
 #endif
 	_sourceWidth(celObj._width) {
-		byte *resource = celObj.getResPointer();
+		const byte *resource = celObj.getResPointer();
 		_pixels = resource + READ_SCI11ENDIAN_UINT32(resource + celObj._celHeaderOffset + 24);
 	}
 
@@ -282,7 +282,7 @@ public:
 
 struct READER_Compressed {
 private:
-	byte *_resource;
+	const byte *const _resource;
 	byte _buffer[1024];
 	uint32 _controlOffset;
 	uint32 _dataOffset;
@@ -301,7 +301,7 @@ public:
 	_maxWidth(maxWidth) {
 		assert(maxWidth <= celObj._width);
 
-		byte *celHeader = _resource + celObj._celHeaderOffset;
+		const byte *const celHeader = _resource + celObj._celHeaderOffset;
 		_dataOffset = READ_SCI11ENDIAN_UINT32(celHeader + 24);
 		_uncompressedDataOffset = READ_SCI11ENDIAN_UINT32(celHeader + 28);
 		_controlOffset = READ_SCI11ENDIAN_UINT32(celHeader + 32);
@@ -311,14 +311,14 @@ public:
 		assert(y >= 0 && y < _sourceHeight);
 		if (y != _y) {
 			// compressed data segment for row
-			byte *row = _resource + _dataOffset + READ_SCI11ENDIAN_UINT32(_resource + _controlOffset + y * 4);
+			const byte *row = _resource + _dataOffset + READ_SCI11ENDIAN_UINT32(_resource + _controlOffset + y * 4);
 
 			// uncompressed data segment for row
-			byte *literal = _resource + _uncompressedDataOffset + READ_SCI11ENDIAN_UINT32(_resource + _controlOffset + _sourceHeight * 4 + y * 4);
+			const byte *literal = _resource + _uncompressedDataOffset + READ_SCI11ENDIAN_UINT32(_resource + _controlOffset + _sourceHeight * 4 + y * 4);
 
 			uint8 length;
 			for (int16 i = 0; i < _maxWidth; i += length) {
-				byte controlByte = *row++;
+				const byte controlByte = *row++;
 				length = controlByte;
 
 				// Run-length encoded
@@ -581,7 +581,7 @@ void CelObj::submitPalette() const {
 int CelObj::_nextCacheId = 1;
 CelCache *CelObj::_cache = nullptr;
 
-int CelObj::searchCache(const CelInfo32 &celInfo, int *nextInsertIndex) const {
+int CelObj::searchCache(const CelInfo32 &celInfo, int *const nextInsertIndex) const {
 	*nextInsertIndex = -1;
 	int oldestId = _nextCacheId + 1;
 	int oldestIndex = 0;
@@ -792,26 +792,27 @@ void CelObj::scaleDrawUncompNoMD(Buffer &target, const Ratio &scaleX, const Rati
 #pragma mark CelObjView
 
 int16 CelObjView::getNumLoops(const GuiResourceId viewId) {
-	Resource *resource = g_sci->getResMan()->findResource(ResourceId(kResourceTypeView, viewId), false);
+	const Resource *const resource = g_sci->getResMan()->findResource(ResourceId(kResourceTypeView, viewId), false);
 
 	if (!resource) {
 		return 0;
 	}
 
+	assert(resource->size >= 3);
 	return resource->data[2];
 }
 
 int16 CelObjView::getNumCels(const GuiResourceId viewId, const int16 loopNo) {
-	Resource *resource = g_sci->getResMan()->findResource(ResourceId(kResourceTypeView, viewId), false);
+	const Resource *const resource = g_sci->getResMan()->findResource(ResourceId(kResourceTypeView, viewId), false);
 
 	if (!resource) {
 		return 0;
 	}
 
-	byte *data = resource->data;
+	const byte *const data = resource->data;
 
-	uint16 loopCount = data[2];
-	if (loopNo >= loopCount) {
+	const uint16 loopCount = data[2];
+	if (loopNo >= loopCount || loopNo < 0) {
 		return 0;
 	}
 
@@ -819,10 +820,15 @@ int16 CelObjView::getNumCels(const GuiResourceId viewId, const int16 loopNo) {
 	const uint8 loopHeaderSize = data[12];
 	const uint8 viewHeaderFieldSize = 2;
 
-	byte *loopHeader = data + viewHeaderFieldSize + viewHeaderSize + (loopHeaderSize * loopNo);
+#ifndef NDEBUG
+	const byte *const dataMax = data + resource->size;
+#endif
+	const byte *loopHeader = data + viewHeaderFieldSize + viewHeaderSize + (loopHeaderSize * loopNo);
+	assert(loopHeader + 3 <= dataMax);
 
 	if ((int8)loopHeader[0] != -1) {
 		loopHeader = data + viewHeaderFieldSize + viewHeaderSize + (loopHeaderSize * (int8)loopHeader[0]);
+		assert(loopHeader >= data && loopHeader + 3 <= dataMax);
 	}
 
 	return loopHeader[2];
@@ -838,7 +844,7 @@ CelObjView::CelObjView(const GuiResourceId viewId, const int16 loopNo, const int
 	_transparent = true;
 
 	int cacheInsertIndex;
-	int cacheIndex = searchCache(_info, &cacheInsertIndex);
+	const int cacheIndex = searchCache(_info, &cacheInsertIndex);
 	if (cacheIndex != -1) {
 		CelCacheEntry &entry = (*_cache)[cacheIndex];
 		const CelObjView *const cachedCelObj = dynamic_cast<CelObjView *>(entry.celObj);
@@ -854,15 +860,14 @@ CelObjView::CelObjView(const GuiResourceId viewId, const int16 loopNo, const int
 	// generates view resource metadata for both SCI16 and SCI32
 	// implementations
 
-	Resource *resource = g_sci->getResMan()->findResource(ResourceId(kResourceTypeView, viewId), false);
+	const Resource *const resource = g_sci->getResMan()->findResource(ResourceId(kResourceTypeView, viewId), false);
 
 	// NOTE: SCI2.1/SQ6 just silently returns here.
 	if (!resource) {
-		warning("View resource %d not loaded", viewId);
-		return;
+		error("View resource %d not found", viewId);
 	}
 
-	byte *data = resource->data;
+	const byte *const data = resource->data;
 
 	_scaledWidth = READ_SCI11ENDIAN_UINT16(data + 14);
 	_scaledHeight = READ_SCI11ENDIAN_UINT16(data + 16);
@@ -881,7 +886,7 @@ CelObjView::CelObjView(const GuiResourceId viewId, const int16 loopNo, const int
 		}
 	}
 
-	uint16 loopCount = data[2];
+	const uint16 loopCount = data[2];
 	if (_info.loopNo >= loopCount) {
 		_info.loopNo = loopCount - 1;
 	}
@@ -896,7 +901,7 @@ CelObjView::CelObjView(const GuiResourceId viewId, const int16 loopNo, const int
 	const uint8 loopHeaderSize = data[12];
 	const uint8 viewHeaderFieldSize = 2;
 
-	byte *loopHeader = data + viewHeaderFieldSize + viewHeaderSize + (loopHeaderSize * _info.loopNo);
+	const byte *loopHeader = data + viewHeaderFieldSize + viewHeaderSize + (loopHeaderSize * _info.loopNo);
 
 	if ((int8)loopHeader[0] != -1) {
 		if (loopHeader[1] == 1) {
@@ -911,10 +916,14 @@ CelObjView::CelObjView(const GuiResourceId viewId, const int16 loopNo, const int
 		_info.celNo = celCount - 1;
 	}
 
+	if (_info.celNo < 0) {
+		error("Cel is less than 0!");
+	}
+
 	_hunkPaletteOffset = READ_SCI11ENDIAN_UINT32(data + 8);
 	_celHeaderOffset = READ_SCI11ENDIAN_UINT32(loopHeader + 12) + (data[13] * _info.celNo);
 
-	byte *celHeader = data + _celHeaderOffset;
+	const byte *const celHeader = data + _celHeaderOffset;
 
 	_width = READ_SCI11ENDIAN_UINT16(celHeader);
 	_height = READ_SCI11ENDIAN_UINT16(celHeader + 2);
@@ -943,7 +952,7 @@ CelObjView::CelObjView(const GuiResourceId viewId, const int16 loopNo, const int
 }
 
 bool CelObjView::analyzeUncompressedForRemap() const {
-	byte *pixels = getResPointer() + READ_SCI11ENDIAN_UINT32(getResPointer() + _celHeaderOffset + 24);
+	const byte *pixels = getResPointer() + READ_SCI11ENDIAN_UINT32(getResPointer() + _celHeaderOffset + 24);
 	for (int i = 0; i < _width * _height; ++i) {
 		const byte pixel = pixels[i];
 		if (
@@ -960,7 +969,7 @@ bool CelObjView::analyzeUncompressedForRemap() const {
 bool CelObjView::analyzeForRemap() const {
 	READER_Compressed reader(*this, _width);
 	for (int y = 0; y < _height; y++) {
-		const byte *curRow = reader.getRow(y);
+		const byte *const curRow = reader.getRow(y);
 		for (int x = 0; x < _width; x++) {
 			const byte pixel = curRow[x];
 			if (
@@ -985,7 +994,7 @@ CelObjView *CelObjView::duplicate() const {
 }
 
 byte *CelObjView::getResPointer() const {
-	const Resource *const resource = g_sci->getResMan()->findResource(ResourceId(kResourceTypeView, _info.resourceId), false);
+	Resource *const resource = g_sci->getResMan()->findResource(ResourceId(kResourceTypeView, _info.resourceId), false);
 	if (resource == nullptr) {
 		error("Failed to load view %d from resource manager", _info.resourceId);
 	}
@@ -1006,7 +1015,7 @@ CelObjPic::CelObjPic(const GuiResourceId picId, const int16 celNo) {
 	_remap = false;
 
 	int cacheInsertIndex;
-	int cacheIndex = searchCache(_info, &cacheInsertIndex);
+	const int cacheIndex = searchCache(_info, &cacheInsertIndex);
 	if (cacheIndex != -1) {
 		CelCacheEntry &entry = (*_cache)[cacheIndex];
 		const CelObjPic *const cachedCelObj = dynamic_cast<CelObjPic *>(entry.celObj);
@@ -1018,15 +1027,14 @@ CelObjPic::CelObjPic(const GuiResourceId picId, const int16 celNo) {
 		return;
 	}
 
-	Resource *resource = g_sci->getResMan()->findResource(ResourceId(kResourceTypePic, picId), false);
+	const Resource *const resource = g_sci->getResMan()->findResource(ResourceId(kResourceTypePic, picId), false);
 
 	// NOTE: SCI2.1/SQ6 just silently returns here.
 	if (!resource) {
-		warning("Pic resource %d not loaded", picId);
-		return;
+		error("Pic resource %d not found", picId);
 	}
 
-	byte *data = resource->data;
+	const byte *const data = resource->data;
 
 	_celCount = data[2];
 
@@ -1037,7 +1045,7 @@ CelObjPic::CelObjPic(const GuiResourceId picId, const int16 celNo) {
 	_celHeaderOffset = READ_SCI11ENDIAN_UINT16(data) + (READ_SCI11ENDIAN_UINT16(data + 4) * _info.celNo);
 	_hunkPaletteOffset = READ_SCI11ENDIAN_UINT32(data + 6);
 
-	byte *celHeader = data + _celHeaderOffset;
+	const byte *const celHeader = data + _celHeaderOffset;
 
 	_width = READ_SCI11ENDIAN_UINT16(celHeader);
 	_height = READ_SCI11ENDIAN_UINT16(celHeader + 2);
@@ -1049,8 +1057,8 @@ CelObjPic::CelObjPic(const GuiResourceId picId, const int16 celNo) {
 	_relativePosition.x = (int16)READ_SCI11ENDIAN_UINT16(celHeader + 38);
 	_relativePosition.y = (int16)READ_SCI11ENDIAN_UINT16(celHeader + 40);
 
-	uint16 sizeFlag1 = READ_SCI11ENDIAN_UINT16(data + 10);
-	uint16 sizeFlag2 = READ_SCI11ENDIAN_UINT16(data + 12);
+	const uint16 sizeFlag1 = READ_SCI11ENDIAN_UINT16(data + 10);
+	const uint16 sizeFlag2 = READ_SCI11ENDIAN_UINT16(data + 12);
 
 	if (sizeFlag2) {
 		_scaledWidth = sizeFlag1;
@@ -1069,7 +1077,7 @@ CelObjPic::CelObjPic(const GuiResourceId picId, const int16 celNo) {
 	if (celHeader[10] & 128) {
 		// NOTE: This is correct according to SCI2.1/SQ6/DOS;
 		// the engine re-reads the byte value as a word value
-		uint16 flags = READ_SCI11ENDIAN_UINT16(celHeader + 10);
+		const uint16 flags = READ_SCI11ENDIAN_UINT16(celHeader + 10);
 		_transparent = flags & 1 ? true : false;
 		_remap = flags & 2 ? true : false;
 	} else {
@@ -1084,8 +1092,8 @@ CelObjPic::CelObjPic(const GuiResourceId picId, const int16 celNo) {
 }
 
 bool CelObjPic::analyzeUncompressedForSkip() const {
-	byte *resource = getResPointer();
-	byte *pixels = resource + READ_SCI11ENDIAN_UINT32(resource + _celHeaderOffset + 24);
+	const byte *const resource = getResPointer();
+	const byte *const pixels = resource + READ_SCI11ENDIAN_UINT32(resource + _celHeaderOffset + 24);
 	for (int i = 0; i < _width * _height; ++i) {
 		uint8 pixel = pixels[i];
 		if (pixel == _transparentColor) {
@@ -1097,7 +1105,7 @@ bool CelObjPic::analyzeUncompressedForSkip() const {
 }
 
 void CelObjPic::draw(Buffer &target, const Common::Rect &targetRect, const Common::Point &scaledPosition, const bool mirrorX) {
-	Ratio square;
+	const Ratio square;
 	_drawMirrored = mirrorX;
 	drawTo(target, targetRect, scaledPosition, square, square);
 }
@@ -1125,15 +1133,21 @@ CelObjMem::CelObjMem(const reg_t bitmapObject) {
 	_celHeaderOffset = 0;
 	_transparent = true;
 
-	SciBitmap &bitmap = *g_sci->getEngineState()->_segMan->lookupBitmap(bitmapObject);
-	_width = bitmap.getWidth();
-	_height = bitmap.getHeight();
-	_displace = bitmap.getDisplace();
-	_transparentColor = bitmap.getSkipColor();
-	_scaledWidth = bitmap.getScaledWidth();
-	_scaledHeight = bitmap.getScaledHeight();
-	_hunkPaletteOffset = bitmap.getHunkPaletteOffset();
-	_remap = bitmap.getRemap();
+	SciBitmap *bitmap = g_sci->getEngineState()->_segMan->lookupBitmap(bitmapObject);
+
+	// NOTE: SSCI did no error checking here at all.
+	if (!bitmap) {
+		error("Bitmap %04x:%04x not found", PRINT_REG(bitmapObject));
+	}
+
+	_width = bitmap->getWidth();
+	_height = bitmap->getHeight();
+	_displace = bitmap->getDisplace();
+	_transparentColor = bitmap->getSkipColor();
+	_scaledWidth = bitmap->getScaledWidth();
+	_scaledHeight = bitmap->getScaledHeight();
+	_hunkPaletteOffset = bitmap->getHunkPaletteOffset();
+	_remap = bitmap->getRemap();
 }
 
 CelObjMem *CelObjMem::duplicate() const {
diff --git a/engines/sci/graphics/celobj32.h b/engines/sci/graphics/celobj32.h
index e1751deb64..e58fb50c98 100644
--- a/engines/sci/graphics/celobj32.h
+++ b/engines/sci/graphics/celobj32.h
@@ -400,7 +400,7 @@ public:
 	 * Reads the pixel at the given coordinates. This method
 	 * is valid only for CelObjView and CelObjPic.
 	 */
-	virtual uint8 readPixel(uint16 x, uint16 y, bool mirrorX) const;
+	virtual uint8 readPixel(const uint16 x, const uint16 y, const bool mirrorX) const;
 
 	/**
 	 * Submits the palette from this cel to the palette