fix: seqLoadStatus OOB reads/writes for custom SAF sequences (#5706) - #6917
Open
bassdr wants to merge 1 commit into
Open
fix: seqLoadStatus OOB reads/writes for custom SAF sequences (#5706)#6917bassdr wants to merge 1 commit into
bassdr wants to merge 1 commit into
Conversation
Custom SAF sequence IDs can exceed sequenceMapSize, causing out-of-bounds reads/writes on the seqLoadStatus byte array (sized exactly sequenceMapSize). This is the direct cause of intermittent battle music failure with BGM packs (issue HarbourMasters#5706): on the second encounter, AudioLoad_SyncLoadSeq reads a garbage value from heap memory past seqLoadStatus[] and may find 1 (loading in progress), causing it to return NULL early — the sequence player is never initialized and no battle music plays. Symmetric fix to the fontLoadStatus guards in PR HarbourMasters#6916: - AudioLoad_IsSeqLoadComplete: return true for OOB seqIds (custom SAF sequences are resource-manager-backed, not in the async-load status table) - AudioLoad_SetSeqLoadStatus: skip update for seqId >= sequenceMapSize - AudioLoad_SyncLoadSeq: skip the in-progress check for OOB seqIds Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
serprex
self-requested a review
July 13, 2026 02:27
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
1 participant
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.
Root cause
gAudioContext.seqLoadStatusis amalloc(sequenceMapSize)byte array — exactlysequenceMapSizeentries. Custom SAF sequences are assigned IDs in the range[seqListSize, sequenceMapSize + 0xF). When a sequence player is initialized viaAudioLoad_SyncInitSeqPlayer,seqPlayer->seqIdis set to the resolved custom ID, which can exceedsequenceMapSize.Three OOB paths were hit every time a custom-BGM battle music track finished:
AudioSeq_SequencePlayerDisable→AudioLoad_SetSeqLoadStatus(seqId, 3)seqLoadStatus[]AudioLoad_SyncLoadSeq:seqLoadStatus[seqId] == 1AudioSeqPlayer_Update→AudioLoad_IsSeqLoadComplete(seqId)The write is deterministic (value 3) but the target heap address varies with allocation history, explaining the intermittent behavior: on the first encounter the heap slot is typically not 1; after subsequent audio operations it may be, silently aborting the battle music initialization.
Fix
Symmetric to the
fontLoadStatusguards in PR #6916:AudioLoad_IsSeqLoadComplete: returntrueforseqId >= sequenceMapSize. Custom SAF sequences are backed by the resource manager and are always available; they are not tracked in the async-load status table.AudioLoad_SetSeqLoadStatus: skip the update for out-of-range IDs.AudioLoad_SyncLoadSeq: skip the in-progress check for out-of-range IDs (cannot be "loading" if not in the table).Test plan
Fixes #5706.
Related: #6916 (same class of bug on fontLoadStatus).
🤖 Generated with Claude Code
Build Artifacts