[Scummvm-git-logs] scummvm master -> c65d64100ec42ad0a15de7d3f904b5374439f042

[athrxx]

This automated email contains information about 1 new commit which have been
pushed to the 'scummvm' repo located at https://github.com/scummvm/scummvm .

Summary:
c65d64100e SCUMM: Prevent an OOB _extraBoxFlags access in Actor::remapDirection()


Commit: c65d64100ec42ad0a15de7d3f904b5374439f042
    https://github.com/scummvm/scummvm/commit/c65d64100ec42ad0a15de7d3f904b5374439f042
Author: Donovan Watteau (contrib at dwatteau.fr)
Date: 2022-09-30T14:24:44+02:00

Commit Message:
SCUMM: Prevent an OOB _extraBoxFlags access in Actor::remapDirection()

_walkbox may be equal to kOldInvalidBox, which is 255, but then
_extraBoxFlags[_walkbox] would be dereferenced while _extraBoxFlags only
has 65 elements.

Found with UBSan in Loom Talkie, when the leaf falls from the tree.

Changed paths:
    engines/scumm/actor.cpp
    engines/scumm/actor.h
    engines/scumm/boxes.cpp


diff --git a/engines/scumm/actor.cpp b/engines/scumm/actor.cpp
index d0252794b92..07fd09c647c 100644
--- a/engines/scumm/actor.cpp
+++ b/engines/scumm/actor.cpp
@@ -1372,16 +1372,19 @@ int Actor::remapDirection(int dir, bool is_walking) {
 	// actor is in the current room anyway.
 
 	if (!_ignoreBoxes || _vm->_game.id == GID_LOOM) {
-		specdir = _vm->_extraBoxFlags[_walkbox];
-		if (specdir) {
-			if (specdir & 0x8000) {
-				dir = specdir & 0x3FFF;
-			} else {
-				specdir = specdir & 0x3FFF;
-				if (specdir - 90 < dir && dir < specdir + 90)
-					dir = specdir;
-				else
-					dir = specdir + 180;
+		if (_walkbox != kOldInvalidBox) {
+			assert(_walkbox < ARRAYSIZE(_vm->_extraBoxFlags));
+			specdir = _vm->_extraBoxFlags[_walkbox];
+			if (specdir) {
+				if (specdir & 0x8000) {
+					dir = specdir & 0x3FFF;
+				} else {
+					specdir = specdir & 0x3FFF;
+					if (specdir - 90 < dir && dir < specdir + 90)
+						dir = specdir;
+					else
+						dir = specdir + 180;
+				}
 			}
 		}
 
diff --git a/engines/scumm/actor.h b/engines/scumm/actor.h
index e87c758a88f..e4415ecd6e0 100644
--- a/engines/scumm/actor.h
+++ b/engines/scumm/actor.h
@@ -76,7 +76,7 @@ struct AdjustBoxResult {	/* Result type of AdjustBox functions */
 };
 
 enum {
-	kOldInvalidBox = 255,	// For small header games
+	kOldInvalidBox = 255,	// For GF_SMALL_HEADER games
 	kNewInvalidBox = 0
 };
 
diff --git a/engines/scumm/boxes.cpp b/engines/scumm/boxes.cpp
index 44a7be6d346..51ea3be4e1f 100644
--- a/engines/scumm/boxes.cpp
+++ b/engines/scumm/boxes.cpp
@@ -165,7 +165,7 @@ static Common::Point closestPtOnLine(const Common::Point &lineStart, const Commo
 byte ScummEngine::getMaskFromBox(int box) {
 	// WORKAROUND for bug #791 and #897. This appears to have been a
 	// long standing bug in the original engine?
-	if (_game.version <= 3 && box == 255)
+	if (_game.version <= 3 && box == kOldInvalidBox)
 		return 1;
 
 	Box *ptr = getBoxBaseAddr(box);
@@ -453,7 +453,7 @@ byte ScummEngine::getNumBoxes() {
 
 Box *ScummEngine::getBoxBaseAddr(int box) {
 	byte *ptr = getResourceAddress(rtMatrix, 2);
-	if (!ptr || box == 255)
+	if (!ptr || box == kOldInvalidBox)
 		return nullptr;
 
 	// WORKAROUND: The NES version of Maniac Mansion attempts to set flags for boxes 2-4