Skip to content

Commit 43f09cd

Browse files
committed
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.
1 parent aef622f commit 43f09cd

File tree

2 files changed

+7
-4
lines changed

2 files changed

+7
-4
lines changed

Diff for: src/debug.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -297,9 +297,9 @@ int getBank(int offs)
297297
//NSF data is easy to overflow the return on.
298298
//Anything over FFFFF will kill it.
299299
if (GameInfo && GameInfo->type == GIT_NSF) {
300-
int addr = GetNesFileAddress(offs) - NES_HEADER_SIZE;
300+
int addr = GetNesFileAddress(offs);
301301

302-
return addr != -1 ? addr / 0x1000 : -1;
302+
return addr != -1 ? (addr - NES_HEADER_SIZE) / 0x1000 : -1;
303303
}
304304

305305
return ((offs >= 0x6000) && (offs <= 0xFFFF)) ? GetPRGBank(offs) : -1;

Diff for: src/drivers/win/debugger.cpp

+5-2
Original file line numberDiff line numberDiff line change
@@ -388,8 +388,11 @@ INT_PTR CALLBACK AddbpCallB(HWND hwndDlg, UINT uMsg, WPARAM wParam, LPARAM lPara
388388
sprintf(str, "%04X", (unsigned int)lParam);
389389
SetDlgItemText(hwndDlg,IDC_ADDBP_ADDR_START,str);
390390
// also set the condition to only break at this Bank
391-
sprintf(str, "K==#%02X", getBank(lParam));
392-
SetDlgItemText(hwndDlg, IDC_ADDBP_CONDITION, str);
391+
auto bank = getBank(lParam);
392+
if (bank > -1) {
393+
sprintf(str, "K==#%02X", bank);
394+
SetDlgItemText(hwndDlg, IDC_ADDBP_CONDITION, str);
395+
}
393396
}
394397
}
395398
break;

0 commit comments

Comments
 (0)