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: f980ec2bc7/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.
This commit is contained in:
Jay Oster 2024-09-17 17:42:57 -07:00
parent aef622f87e
commit 43f09cd72d
2 changed files with 7 additions and 4 deletions

View File

@ -297,9 +297,9 @@ int getBank(int offs)
//NSF data is easy to overflow the return on. //NSF data is easy to overflow the return on.
//Anything over FFFFF will kill it. //Anything over FFFFF will kill it.
if (GameInfo && GameInfo->type == GIT_NSF) { 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; return ((offs >= 0x6000) && (offs <= 0xFFFF)) ? GetPRGBank(offs) : -1;

View File

@ -388,10 +388,13 @@ INT_PTR CALLBACK AddbpCallB(HWND hwndDlg, UINT uMsg, WPARAM wParam, LPARAM lPara
sprintf(str, "%04X", (unsigned int)lParam); sprintf(str, "%04X", (unsigned int)lParam);
SetDlgItemText(hwndDlg,IDC_ADDBP_ADDR_START,str); SetDlgItemText(hwndDlg,IDC_ADDBP_ADDR_START,str);
// also set the condition to only break at this Bank // also set the condition to only break at this Bank
sprintf(str, "K==#%02X", getBank(lParam)); auto bank = getBank(lParam);
if (bank > -1) {
sprintf(str, "K==#%02X", bank);
SetDlgItemText(hwndDlg, IDC_ADDBP_CONDITION, str); SetDlgItemText(hwndDlg, IDC_ADDBP_CONDITION, str);
} }
} }
}
break; break;
case WM_CLOSE: case WM_CLOSE:
case WM_QUIT: case WM_QUIT: