Skip to content

Commit

Permalink
Fix stack corruption in debugger
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
parasyte committed Sep 19, 2024
1 parent aef622f commit ab1ef8c
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 4 deletions.
4 changes: 2 additions & 2 deletions src/debug.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
7 changes: 5 additions & 2 deletions src/drivers/win/debugger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit ab1ef8c

Please sign in to comment.