synthio: a freed track buffer, a ring modulation sign flip, reads pas… - #11370
Open
peterbay wants to merge 1 commit into
Open
synthio: a freed track buffer, a ring modulation sign flip, reads pas…#11370peterbay wants to merge 1 commit into
peterbay wants to merge 1 commit into
Conversation
…t the end from_file freed the track buffer it had just handed to MidiTrack, which does not copy it: it keeps the pointer and parses it as the track plays, so everything after the first pause was decoded from memory that had been returned to the allocator. Ring modulation of two troughs gave -32768 * -32768 / 32768, which is +32768 and was stored into an int16_t as -32768, a full scale sign flip. It is computed in an int32_t and clamped now. dds_rate is worked out from the waveform loop's length, but the nyquist test compared it with the whole waveform's, so a frequency past nyquist for the loop was let through and aliased. lim is the loop end shifted, so accum == lim indexes one int16_t past the end and the test has to be >=; the wrap beside it did not land inside the loop either. parse_note recorded a stream error and then fell through into two reads with pos already at len, because recording the error sets pos = len. The event loop had no guard of its own, so a one byte track reached the read. MidiTrack's tempo and Synthesizer's sample_rate are divisors and were not checked in the constructors.
Author
|
Testing and diagnostic script. |
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.
Code written by Claude Code, guided and corrected by @peterbay.
The problem
Six defects in
synthio: a buffer freed while it is still being read, a full scale sign flip in ring modulation, two reads past the end of a buffer, and two divisors that were never checked.The changes
from_filefreed the track it had just handed over.common_hal_synthio_miditrack_constructdoes not copy the buffer — it keeps the pointer and parses it as the track plays, so everything after the first pause was decoded from memory that had been returned to the allocator. Them_freeis removed; the buffer is owned by theMidiTrackfrom that point on.Ring modulation of two troughs came out as a full scale spike of the wrong sign.
-32768 * -32768 / 32768is+32768, which was stored into anint16_tand read back as-32768. Widened toint32_tand clamped at 32767.The nyquist test used the whole waveform's length, not the loop's.
dds_rateis worked out fromwaveform_length - waveform_start, so the limit has to be(lim - offset) / 2. With a 256 sample loop inside a 512 sample waveform at 8 kHz the threshold came out at 8 kHz rather than the real 4 kHz, and everything between the two was let through and aliased.The accumulator could be left outside the loop.
limis the loop end shifted, soaccum == limindexeswaveform[waveform_length], oneint16_tpast the end; the comparison has to be>=. The wrap alongside it,accum % lim + offset, does not land in[offset, lim)either — it isoffset + (accum - offset) % (lim - offset). Both are reached when a note's waveform or loop points are changed while it sounds.A MIDI message that runs off the end of the track was parsed anyway.
parse_noterecorded the stream error and then fell through into its two reads withposalready atlen, becauserecord_midi_stream_errorsetspos = len. It returns after recording now. The event loop had no guard of its own either, sosynthio.MidiTrack(b"\x00", ...)reached the read; it breaks whenposis at the end.MidiTrack(tempo=0)andSynthesizer(sample_rate=0)were accepted. Both are divisors further in, and inMidiTrack's case only the setter checked it, not the constructor.Testing
Seeed XIAO nRF52840 Sense, on two builds differing only by these changes. Notes were rendered into a WAV in RAM with
audiofilewriter.AudioFileWriter, so the samples could be compared rather than listened to.Synthesizer(sample_rate=0)ValueError: sample_rate must be >= 1MidiTrack(tempo=0)ValueError: tempo must be >= 1The ring modulation row is the sign flip in full: the whole rendered note is negative before the change and positive after it.
Two of the six are not covered by that table. Reading past the end of a MIDI track returns whatever lies next in the heap, and on this board that did not change what was played. The freed track buffer is the same kind of thing: the allocator did not hand that block back during the window the test left it, so the output was the same either way. Both are clear enough in the code — a pointer kept after the buffer is freed, and a read at
poswhenpos == len.