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

[sluicebox]

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:
cb0db51091 AGI: Fix closing window when overlapping with menu


Commit: cb0db51091ddcb67de95bfaa5e0f94a4f619db6d
    https://github.com/scummvm/scummvm/commit/cb0db51091ddcb67de95bfaa5e0f94a4f619db6d
Author: sluicebox (22204938+sluicebox at users.noreply.github.com)
Date: 2024-07-05T09:55:51-07:00

Commit Message:
AGI: Fix closing window when overlapping with menu

Fixes out of bounds memory access when closing a window that overlaps
with the menu bar at the top of the screen. This crashed at least some
release builds, including Windows release 2.8.1.

Mixed-Up Mother Goose is the only game known to draw a text window over
the menu bar. When I recently added support for this, I missed that the
code for closing the window needed to be updated too:
bc8550ce02a036e3757d817e963e038463844412

Fixes bug #15241

Big thanks to @hugoarpin for reporting this and testing the fix.

Changed paths:
    engines/agi/graphics.cpp
    engines/agi/graphics.h
    engines/agi/text.cpp


diff --git a/engines/agi/graphics.cpp b/engines/agi/graphics.cpp
index 867cd048b0c..e8c457b9aa1 100644
--- a/engines/agi/graphics.cpp
+++ b/engines/agi/graphics.cpp
@@ -539,12 +539,7 @@ void GfxMgr::render_Block(int16 x, int16 y, int16 width, int16 height, bool copy
 // logically dead or disabled. Is the purpose to adjust out-of-bounds coordinates
 // so that they fit within boundaries, or is it to identify out-of-bounds
 // coordinates so that the entire drawing operation can be rejected?
-bool GfxMgr::render_Clip(int16 &x, int16 &y, int16 &width, int16 &height, int16 clipAgainstWidth, int16 clipAgainstHeight) {
-	// Allow rendering all the way to the top of the visual screen, even if there
-	// is a menu bar. MMMG nursery rhyme message boxes appear over the menu bar
-	// when the scripts pass a y-coordinate of zero to print.at(). Bug #13820
-	int16 minY = 0 - _renderStartDisplayOffsetY;
-
+bool GfxMgr::render_Clip(int16 &x, int16 &y, int16 &width, int16 &height, const int16 minY, const int16 clipAgainstWidth, const int16 clipAgainstHeight) {
 	if ((x >= clipAgainstWidth) || ((x + width - 1) < 0) ||
 	        (y < minY) || ((y + (height - 1)) >= clipAgainstHeight)) {
 		return false;
@@ -978,7 +973,11 @@ void GfxMgr::block_restore(int16 x, int16 y, int16 width, int16 height, byte *bu
 //            Going beyond 160x168 will result in messageboxes not getting fully removed
 //            In KQ4's case, the scripts clear the screen that's why it works.
 void GfxMgr::drawBox(int16 x, int16 y, int16 width, int16 height, byte backgroundColor, byte lineColor) {
-	if (!render_Clip(x, y, width, height, VISUAL_WIDTH, VISUAL_HEIGHT - _renderStartVisualOffsetY)) {
+	// Allow rendering all the way to the top of the visual screen, even if there
+	// is a menu bar. MMMG nursery rhyme message boxes appear over the menu bar
+	// when the scripts pass a y-coordinate of zero to print.at(). Bug #13820
+	const int16 minY = 0 - _renderStartDisplayOffsetY;
+	if (!render_Clip(x, y, width, height, minY, VISUAL_WIDTH, VISUAL_HEIGHT - _renderStartVisualOffsetY)) {
 		warning("drawBox ignored by clipping. x: %d, y: %d, w: %d, h: %d", x, y, width, height);
 		return;
 	}
diff --git a/engines/agi/graphics.h b/engines/agi/graphics.h
index 8ed36ce5ebf..9ef4e438750 100644
--- a/engines/agi/graphics.h
+++ b/engines/agi/graphics.h
@@ -173,7 +173,7 @@ public:
 	byte getCGAMixtureColor(byte color);
 
 	void render_Block(int16 x, int16 y, int16 width, int16 height, bool copyToScreen = true);
-	bool render_Clip(int16 &x, int16 &y, int16 &width, int16 &height, int16 clipAgainstWidth = SCRIPT_WIDTH, int16 clipAgainstHeight = SCRIPT_HEIGHT);
+	bool render_Clip(int16 &x, int16 &y, int16 &width, int16 &height, const int16 minY = 0, const int16 clipAgainstWidth = SCRIPT_WIDTH, const int16 clipAgainstHeight = SCRIPT_HEIGHT);
 
 private:
 	void render_BlockEGA(int16 x, int16 y, int16 width, int16 height);
diff --git a/engines/agi/text.cpp b/engines/agi/text.cpp
index 58dbbb1c4a9..06773598849 100644
--- a/engines/agi/text.cpp
+++ b/engines/agi/text.cpp
@@ -548,7 +548,16 @@ bool TextMgr::isMouseWithinMessageBox() {
 
 void TextMgr::closeWindow() {
 	if (_messageState.window_Active) {
-		_gfx->render_Block(_messageState.backgroundPos_x, _messageState.backgroundPos_y, _messageState.backgroundSize_Width, _messageState.backgroundSize_Height);
+		// Close the window by copying the game screen to the display screen.
+		// Our graphics code was designed with the assumption that the window would
+		// always be contained within the game screen, but MUMG nursery rhymes pass
+		// y=0 to print.at to place the text at the top of the game screen with the
+		// window border over the menu bar. We now support this, but the background
+		// position is in game screen coordinates, so it will have a negative y value
+		// that we must limit to zero before copying. Bugs #13820, #15241
+		const int16 x = _messageState.backgroundPos_x;
+		const int16 y = MAX<int16>(0, _messageState.backgroundPos_y);
+		_gfx->render_Block(x, y, _messageState.backgroundSize_Width, _messageState.backgroundSize_Height);
 	}
 	_messageState.dialogue_Open = false;
 	_messageState.window_Active = false;