From ab1ef8cac6719ba5676e61875f800c535d719610 Mon Sep 17 00:00:00 2001
From: Jay Oster <jay@kodewerx.org>
Date: Tue, 17 Sep 2024 17:42:57 -0700
Subject: [PATCH] Fix stack corruption in debugger

Corrupt the stack with the following process on a release build (prior
to this commit):

- Open FCEUX, do NOT load a ROM.
- Open the debugger window.
- Resize the debugger window to force it to refresh the disassembler.
- Double click on any address that is not $0000.
- The Add Breakpoint window will open with the condition string filled
  with `K==#FFFFFFFF`, which is at least 13 characters long.
- The `str` array that this string is written to only has capacity for 8
  characters.
- Whoops!

For debug builds, the process is the same, except you don't have to
force the disassembler to refresh by resizing the window. The
debugger window will open with the disassembly ready to click on.

This commit fixes a bug in the original `getBank()` implementation when
`GetNesFileAddress()` returns -1.

See: https://github.com/TASEmulators/fceux/blob/f980ec2bc7dc962f6cd76b9ae3131f2eb902c9e7/src/debug.cpp#L303-L307

`addr` will be -17 in this error condition after the iNES header size is
subtracted. This causes the following error checks to fail and weird
integer arithmetic (specifically `-17 / (1 << 14)` is 0!) then returns 0
to the caller, indicating a successful result for bank number 0.

With the fix, `getBank()` now properly returns -1 and causes the stack
corruption with unrelated code as described above. This commit adds
proper error handling to the code in question.

Additionally, the previous commit also kept the original
`-17 / 0x1000 == 0` behavior for NSFs. That is now corrected in this
commit; `getBank()` always returns -1 for errors instead of integer
divisions truncating negative results to 0.
---
 src/debug.cpp                | 4 ++--
 src/drivers/win/debugger.cpp | 7 +++++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/debug.cpp b/src/debug.cpp
index c0e1668cf..84e1267fb 100644
--- a/src/debug.cpp
+++ b/src/debug.cpp
@@ -297,9 +297,9 @@ int getBank(int offs)
 	//NSF data is easy to overflow the return on.
 	//Anything over FFFFF will kill it.
 	if (GameInfo && GameInfo->type == GIT_NSF) {
-		int addr = GetNesFileAddress(offs) - NES_HEADER_SIZE;
+		int addr = GetNesFileAddress(offs);
 
-		return addr != -1 ? addr / 0x1000 : -1;
+		return addr != -1 ? (addr - NES_HEADER_SIZE) / 0x1000 : -1;
 	}
 
 	return ((offs >= 0x6000) && (offs <= 0xFFFF)) ? GetPRGBank(offs) : -1;
diff --git a/src/drivers/win/debugger.cpp b/src/drivers/win/debugger.cpp
index 2ae4c47dd..e6f4139a8 100644
--- a/src/drivers/win/debugger.cpp
+++ b/src/drivers/win/debugger.cpp
@@ -388,8 +388,11 @@ INT_PTR CALLBACK AddbpCallB(HWND hwndDlg, UINT uMsg, WPARAM wParam, LPARAM lPara
 					sprintf(str, "%04X", (unsigned int)lParam);
 					SetDlgItemText(hwndDlg,IDC_ADDBP_ADDR_START,str);
 					// also set the condition to only break at this Bank
-					sprintf(str, "K==#%02X", getBank(lParam));
-					SetDlgItemText(hwndDlg, IDC_ADDBP_CONDITION, str);
+					auto bank = getBank(lParam);
+					if (bank > -1) {
+						sprintf(str, "K==#%02X", bank);
+						SetDlgItemText(hwndDlg, IDC_ADDBP_CONDITION, str);
+					}
 				}
 			}
 			break;