Merged revisions 32668-32669,32676,32687-32689,32693,32695,32698-32701,32705 via svnmerge from

https://scummvm.svn.sourceforge.net/svnroot/scummvm/scummvm/trunk

svn-id: r32720

3 files changed, 100 insertions, 43 deletions

diff --git a/engines/cine/part.cpp b/engines/cine/part.cpp
index 535bf841d5..b39f1eff7d 100644
--- a/engines/cine/part.cpp
+++ b/engines/cine/part.cpp
@@ -128,7 +128,7 @@ void CineEngine::readVolCnf() {
 	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 *readBundleFile(int16 foundFileIdx) {
 		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);
diff --git a/engines/cine/unpack.cpp b/engines/cine/unpack.cpp
index 364331e439..dcd3181242 100644
--- a/engines/cine/unpack.cpp
+++ b/engines/cine/unpack.cpp
@@ -31,13 +31,17 @@
 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;
 }
 
-int CineUnpacker::rcr(int inputCarry) {
-	int outputCarry = (_chunk32b & 1);
+uint CineUnpacker::rcr(bool inputCarry) {
+	uint outputCarry = (_chunk32b & 1);
 	_chunk32b >>= 1;
 	if (inputCarry) {
 		_chunk32b |= 0x80000000;
@@ -45,20 +49,20 @@ int CineUnpacker::rcr(int inputCarry) {
 	return outputCarry;
 }
 
-int CineUnpacker::nextBit() {
-	int carry = rcr(0);
+uint CineUnpacker::nextBit() {
+	uint carry = rcr(false);
 	// Normally if the chunk becomes zero then the carry is one as
 	// the end of chunk marker is always the last to be shifted out.
 	if (_chunk32b == 0) {
 		_chunk32b = readSource();
 		_crc ^= _chunk32b;
-		carry = rcr(1); // Put the end of chunk marker in the most significant bit
+		carry = rcr(true); // Put the end of chunk marker in the most significant bit
 	}
 	return carry;
 }
 
-uint16 CineUnpacker::getBits(byte numBits) {
-	uint16 c = 0;
+uint CineUnpacker::getBits(uint numBits) {
+	uint c = 0;
 	while (numBits--) {
 		c <<= 1;
 		c |= nextBit();
@@ -66,30 +70,45 @@ uint16 CineUnpacker::getBits(byte numBits) {
 	return c;
 }
 
-void CineUnpacker::unpackRawBytes(uint16 numBytes) {
-	_datasize -= numBytes;
+void CineUnpacker::unpackRawBytes(uint 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;
 	}
 }
 
-void CineUnpacker::copyRelocatedBytes(uint16 offset, uint16 numBytes) {
-	_datasize -= numBytes;
+void CineUnpacker::copyRelocatedBytes(uint offset, uint 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)
@@ -101,30 +120,30 @@ bool CineUnpacker::unpack(byte *dst, const byte *src, int srcLen) {
 		*/
 		if (!nextBit()) { // 0...
 			if (!nextBit()) { // 0 0
-				uint16 numBytes = getBits(3) + 1;
+				uint numBytes = getBits(3) + 1;
 				unpackRawBytes(numBytes);
 			} else { // 0 1
-				uint16 numBytes = 2;
-				uint16 offset   = getBits(8);
+				uint numBytes = 2;
+				uint offset   = getBits(8);
 				copyRelocatedBytes(offset, numBytes);
 			}
 		} else { // 1...
-			uint16 c = getBits(2);
+			uint c = getBits(2);
 			if (c == 3) { // 1 1 1
-				uint16 numBytes = getBits(8) + 9;
+				uint numBytes = getBits(8) + 9;
 				unpackRawBytes(numBytes);
 			} else if (c < 2) { // 1 0 x
-				uint16 numBytes = c + 3;
-				uint16 offset   = getBits(c + 9);
+				uint numBytes = c + 3;
+				uint offset   = getBits(c + 9);
 				copyRelocatedBytes(offset, numBytes);
 			} else { // 1 1 0
-				uint16 numBytes = getBits(8) + 1;
-				uint16 offset   = getBits(12);
+				uint numBytes = getBits(8) + 1;
+				uint offset   = getBits(12);
 				copyRelocatedBytes(offset, numBytes);
 			}
 		}
-	} while (_datasize > 0 && _src >= src - 4);
-	return _crc == 0;
+	}
+	return !_error && (_crc == 0);
 }
 
 } // End of namespace Cine
diff --git a/engines/cine/unpack.h b/engines/cine/unpack.h
index f0a7ee3804..e16cb594a9 100644
--- a/engines/cine/unpack.h
+++ b/engines/cine/unpack.h
@@ -39,41 +39,77 @@ namespace Cine {
  */
 class CineUnpacker {
 public:
-	/** Returns true if unpacking was successful, otherwise false. */
-	bool unpack(byte *dst, const byte *src, int srcLen);
+	/**
+	 * Unpacks packed data from the source buffer to the destination buffer.
+	 * @warning Do NOT call this on data that is not packed.
+	 * @note Source and destination buffer pointers can be the same as long as there's space for the unpacked data.
+	 * @param src Pointer to the source buffer.
+	 * @param srcLen Length of the source buffer.
+	 * @param dst Pointer to the destination buffer.
+	 * @param dstLen Length of the destination buffer.
+	 * @return True if no errors were detected in the source data and unpacking was successful, otherwise false.
+	 */
+	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. */
+	/**
+	 * Reads an unsigned big endian 32-bit integer from the source stream and goes backwards 4 bytes.
+	 * @return If the operation is valid, an unsigned big endian 32-bit integer read from the source stream.
+	 * @return If the operation is invalid, zero.
+	 * @note Sets internal error state if the read operation would be out of source bounds.
+	 */
 	uint32 readSource();
 
 	/**
 	 * Shifts the current internal 32-bit chunk to the right by one.
 	 * Puts input carry into internal chunk's topmost (i.e. leftmost) bit.
-	 * Returns the least significant bit that was shifted out.
+	 * @return The least significant bit that was shifted out from the chunk.
 	 */
-	int rcr(int inputCarry);
-	int nextBit();
-	uint16 getBits(byte numBits);
+	uint rcr(bool inputCarry);
+
+	/**
+	 * Get the next bit from the source stream.
+	 * @note Changes the bit position in the source stream.
+	 * @return The next bit from the source stream.
+	 */
+	uint nextBit();
+
+	/**
+	 * Get bits from the source stream.
+	 * @note Changes the bit position in the source stream.
+	 * @param numBits Number of bits to read from the source stream.	 
+	 * @return Integer value consisting of the bits read from the source stream (In range [0, (2 ** numBits) - 1]).
+	 * @return Later the bit was read from the source, the less significant it is in the return value.
+	 */
+	uint getBits(uint numBits);
 
 	/**
 	 * Copy raw bytes from the input stream and write them to the destination stream.
 	 * This is used when no adequately long match is found in the sliding window.
+	 * @note Sets internal error state if the operation would be out of bounds.
 	 * @param numBytes Amount of bytes to copy from the input stream
 	 */
-	void unpackRawBytes(uint16 numBytes);
+	void unpackRawBytes(uint numBytes);
 
 	/**
 	 * Copy bytes from the sliding window in the destination buffer.
 	 * This is used when a match of two bytes or longer is found.
+	 * @note Sets internal error state if the operation would be out of bounds.
 	 * @param offset Offset in the sliding window
 	 * @param numBytes Amount of bytes to copy
 	 */
-	void copyRelocatedBytes(uint16 offset, uint16 numBytes);
+	void copyRelocatedBytes(uint offset, uint numBytes);
 private:
-	int _datasize;    //!< Bytes left to write into the unpacked data stream
-	uint32 _crc;      //!< Error-detecting code
-	uint32 _chunk32b; //!< The current internal 32-bit chunk
-	byte *_dst;       //!< Destination buffer pointer
-	const byte *_src; //!< Source buffer pointer
+	uint32 _crc;      //!< Error-detecting code (This should be zero after successful unpacking)
+	uint32 _chunk32b; //!< The current internal 32-bit chunk of source data
+	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