Fix sound Channel{1-4}.loadState slot index typo causing OOB on second loadState#377
Open
fsegouin wants to merge 1 commit into
Open
Fix sound Channel{1-4}.loadState slot index typo causing OOB on second loadState#377fsegouin wants to merge 1 commit into
fsegouin wants to merge 1 commit into
Conversation
Each sound channel's static loadState() passed the runtime cycleCounter value to getSaveStateMemoryOffset() as the save-state slot index, instead of the constant saveStateSlot. After the first loadState() the cycleCounter holds an arbitrary 32-bit value read from a wrong offset, and the next loadState() computes 1024 + 50 * cycleCounter, which can land far past WASM linear memory and trap with "memory access out of bounds". This blocks any caller that loads more than one save state in the same WasmBoy session (e.g. an automated tool looping load-state + input macro to brute-force RNG outcomes). The matching saveState() in each file already uses saveStateSlot, so this restores symmetry between save and load. All four channels had the same copy-paste typo and all four are fixed.
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A copy-paste typo in each sound channel's
static loadState()method passes the runtimecycleCountervalue togetSaveStateMemoryOffset()as the save-state slot index, instead of the constantsaveStateSlot.For example, in
core/sound/channel1.ts:The matching
saveState()in each file already usessaveStateSlot(e.g.store<i32>(getSaveStateMemoryOffset(0x00, Channel1.saveStateSlot), Channel1.cycleCounter);), so this PR restores symmetry between save and load — it is not a behavior change, just a fix to a slot index that should always have been the constant.All four sound channels have the same typo and all four are fixed:
core/sound/channel1.ts—Channel1.saveStateSlot = 7core/sound/channel2.ts—Channel2.saveStateSlot = 8core/sound/channel3.ts—Channel3.saveStateSlot = 9core/sound/channel4.ts—Channel4.saveStateSlot = 10Impact
After the first call to the WASM
loadState(), each channel'scycleCounterholds whatever 32-bit value happened to be at offset0x00of an unrelated save-state region (in practice, near the start of the CPU register data). On the nextloadState(),getSaveStateMemoryOffset(0x00, cycleCounter)evaluates to1024 + 50 * cycleCounter, which can land far past the bounds of WASM linear memory and trap with:This blocks any caller that loads more than one save state in the same WasmBoy session — for instance, an automated tool that loops
loadState → run input macro → loadState → run input macroto brute-force RNG outcomes. The first load works; the second crashes the WASM module.Diff
Four single-line changes, all of the same form (
ChannelN.cycleCounter→ChannelN.saveStateSlotin the second argument ofgetSaveStateMemoryOffset).Test plan
saveState()was already broken, not tested).loadState()calls in the same session no longer trap.