[Scummvm-cvs-logs] SF.net SVN: scummvm: [32687] scummvm/trunk/engines/cine

[buddha_ at users.sourceforge.net]

Revision: 32687
          http://scummvm.svn.sourceforge.net/scummvm/?rev=32687&view=rev
Author:   buddha_
Date:     2008-06-12 22:57:07 -0700 (Thu, 12 Jun 2008)

Log Message:
-----------
Made CineUnpacker::unpack more robust & secure. It shouldn't crash now with any input (Before making reading or writing operations they are checked to be in bounds). Also updated some comments and added some error message(s).

Modified Paths:
--------------
    scummvm/trunk/engines/cine/part.cpp
    scummvm/trunk/engines/cine/unpack.cpp
    scummvm/trunk/engines/cine/unpack.h

Modified: scummvm/trunk/engines/cine/part.cpp
===================================================================
--- scummvm/trunk/engines/cine/part.cpp	2008-06-12 23:13:58 UTC (rev 32686)
+++ scummvm/trunk/engines/cine/part.cpp	2008-06-13 05:57:07 UTC (rev 32687)
@@ -128,7 +128,7 @@
 	f.read(buf, packedSize);
 	if (packedSize != unpackedSize) {
 		CineUnpacker cineUnpacker;
-		if (!cineUnpacker.unpack(buf, buf, packedSize)) {
+		if (!cineUnpacker.unpack(buf, packedSize, buf, unpackedSize)) {
 			error("Error while unpacking 'vol.cnf' data");
 		}
 	}
@@ -226,7 +226,9 @@
 		byte *unpackBuffer = (byte *)malloc(partBuffer[foundFileIdx].packedSize);
 		readFromPart(foundFileIdx, unpackBuffer);
 		CineUnpacker cineUnpacker;
-		cineUnpacker.unpack(dataPtr, unpackBuffer, partBuffer[foundFileIdx].packedSize);
+		if (!cineUnpacker.unpack(unpackBuffer, partBuffer[foundFileIdx].packedSize, dataPtr, partBuffer[foundFileIdx].unpackedSize)) {
+			warning("Error unpacking '%s' from bundle file '%s'", partBuffer[foundFileIdx].partName, currentPartName);
+		}
 		free(unpackBuffer);
 	} else {
 		readFromPart(foundFileIdx, dataPtr);

Modified: scummvm/trunk/engines/cine/unpack.cpp
===================================================================
--- scummvm/trunk/engines/cine/unpack.cpp	2008-06-12 23:13:58 UTC (rev 32686)
+++ scummvm/trunk/engines/cine/unpack.cpp	2008-06-13 05:57:07 UTC (rev 32687)
@@ -31,6 +31,10 @@
 namespace Cine {
 
 uint32 CineUnpacker::readSource() {
+	if (_src < _srcBegin || _src + 4 > _srcEnd) {
+		_error = true;
+		return 0; // The source pointer is out of bounds, returning a default value
+	}
 	uint32 value = READ_BE_UINT32(_src);
 	_src -= 4;
 	return value;
@@ -67,7 +71,10 @@
 }
 
 void CineUnpacker::unpackRawBytes(uint16 numBytes) {
-	_datasize -= numBytes;
+	if (_dst >= _dstEnd || _dst - numBytes + 1 < _dstBegin) {
+		_error = true;
+		return; // Destination pointer is out of bounds for this operation
+	}
 	while (numBytes--) {
 		*_dst = (byte)getBits(8);
 		--_dst;
@@ -75,21 +82,33 @@
 }
 
 void CineUnpacker::copyRelocatedBytes(uint16 offset, uint16 numBytes) {
-	_datasize -= numBytes;
+	if (_dst + offset >= _dstEnd || _dst - numBytes + 1 < _dstBegin) {
+		_error = true;
+		return; // Destination pointer is out of bounds for this operation
+	}
 	while (numBytes--) {
 		*_dst = *(_dst + offset);
 		--_dst;
 	}
 }
 
-bool CineUnpacker::unpack(byte *dst, const byte *src, int srcLen) {
-	_src = src + srcLen - 4;
-	_datasize = readSource(); // Unpacked length in bytes
-	_dst = dst + _datasize - 1;
+bool CineUnpacker::unpack(const byte *src, uint srcLen, byte *dst, uint dstLen) {
+	// Initialize variables used for detecting errors during unpacking
+	_error    = false;
+	_srcBegin = src;
+	_srcEnd   = src + srcLen;
+	_dstBegin = dst;
+	_dstEnd   = dst + dstLen;
+
+	// Initialize other variables
+	_src = _srcBegin + srcLen - 4;
+	uint32 unpackedLength = readSource(); // Unpacked length in bytes
+	_dst = _dstBegin + unpackedLength - 1;
 	_crc = readSource();
 	_chunk32b = readSource();
 	_crc ^= _chunk32b;
-	do {
+
+	while (_dst >= _dstBegin && !_error) {
 		/*
 		Bits  => Action:
 		0 0   => unpackRawBytes(3 bits + 1)              i.e. unpackRawBytes(1..9)
@@ -123,8 +142,8 @@
 				copyRelocatedBytes(offset, numBytes);
 			}
 		}
-	} while (_datasize > 0 && _src >= src - 4);
-	return _crc == 0;
+	}
+	return !_error && (_crc == 0);
 }
 
 } // End of namespace Cine

Modified: scummvm/trunk/engines/cine/unpack.h
===================================================================
--- scummvm/trunk/engines/cine/unpack.h	2008-06-12 23:13:58 UTC (rev 32686)
+++ scummvm/trunk/engines/cine/unpack.h	2008-06-13 05:57:07 UTC (rev 32687)
@@ -40,7 +40,7 @@
 class CineUnpacker {
 public:
 	/** Returns true if unpacking was successful, otherwise false. */
-	bool unpack(byte *dst, const byte *src, int srcLen);
+	bool unpack(const byte *src, uint srcLen, byte *dst, uint dstLen);
 private:
 	/** Reads a single big endian 32-bit integer from the source and goes backwards 4 bytes. */
 	uint32 readSource();
@@ -69,11 +69,17 @@
 	 */
 	void copyRelocatedBytes(uint16 offset, uint16 numBytes);
 private:
-	int _datasize;    //!< Bytes left to write into the unpacked data stream
-	uint32 _crc;      //!< Error-detecting code
+	uint32 _crc;      //!< Error-detecting code (This should be zero after successful unpacking)
 	uint32 _chunk32b; //!< The current internal 32-bit chunk
-	byte *_dst;       //!< Destination buffer pointer
-	const byte *_src; //!< Source buffer pointer
+	byte *_dst;       //!< Pointer to the current position in the destination buffer
+	const byte *_src; //!< Pointer to the current position in the source buffer
+
+	// These are used for detecting errors (e.g. out of bounds issues) during unpacking
+	bool _error;           //!< Did an error occur during unpacking?
+	const byte *_srcBegin; //!< Source buffer's beginning
+	const byte *_srcEnd;   //!< Source buffer's end
+	byte *_dstBegin;       //!< Destination buffer's beginning
+	byte *_dstEnd;         //!< Destination buffer's end
 };
 
 } // End of namespace Cine


This was sent by the SourceForge.net collaborative development platform, the world's largest Open Source development site.