Fix unbounded sysex_buffer accumulation in convert_midi_bytes_to_messages#912
Open
94xhn wants to merge 1 commit into
Open
Fix unbounded sysex_buffer accumulation in convert_midi_bytes_to_messages#91294xhn wants to merge 1 commit into
94xhn wants to merge 1 commit into
Conversation
convert_midi_bytes_to_messages() accumulated incoming sysex data bytes into sysex_buffer with no length check: sysex_buffer[sysex_len++] = byte ran unconditionally while sysex_flag was set. sysex_buffer is allocated to exactly MAX_SYSEX_BYTES (16384). A sysex payload longer than that with no intervening 0xF7 terminator or fresh status byte -- a large bulk sysex dump, or simply a corrupted/noisy MIDI stream that drops the terminating F7 -- walks straight past the allocation and corrupts the heap. Cap sysex_len at MAX_SYSEX_BYTES - 1 (not MAX_SYSEX_BYTES) before the accumulation write, dropping any further bytes once the buffer is full. The "- 1" leaves room for the NUL terminator parse_sysex() later writes at sysex_buffer[sysex_len].
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.
Bug
convert_midi_bytes_to_messages()insrc/amy_midi.caccumulates incoming sysex data bytes intosysex_bufferwith no length check:sysex_bufferis allocated to exactlyMAX_SYSEX_BYTES(16384, defined insrc/amy_midi.h).sysex_len/sysex_flagare only reset on an0xF7terminator or a fresh MIDI status byte (0x80-0xF7). A sysex payload longer than 16384 bytes with no intervening terminator — a legitimate large bulk sysex dump, or simply a corrupted/noisy MIDI stream that drops the terminating F7 — writes straight past the allocation and corrupts the heap.parse_sysex()additionally writes a NUL terminator atsysex_buffer[sysex_len], so the cap needs to beMAX_SYSEX_BYTES - 1, notMAX_SYSEX_BYTES, to keep that write in-bounds too.Fix
Verification
Built a guard-page harness (
VirtualAlloc+VirtualProtect PAGE_NOACCESSimmediately after aMAX_SYSEX_BYTES-sized allocation) and ran the accumulation branch verbatim as it appears in the source, both before and after the fix:0xF7) capssysex_lenat 16383, the guard page is never touched, and the subsequent NUL-terminator write also lands in-bounds.Also compiled
src/amy_midi.cstandalone (gcc -c) with the fix — no new warnings.