From 43f09cd72d262eb5050a9aa3fd9368b480a833da 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 (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.
  - (May not be necessary if you have already saved the debugger state
    with a larger-than-default window size.)
- 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!

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 c0e1668c..84e1267f 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 2ae4c47d..e6f4139a 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;