From 904e7a7a552a56e490db9141b510862111b3ebab Mon Sep 17 00:00:00 2001 From: Peter Bay Date: Sun, 13 Sep 2026 22:43:11 +0200 Subject: [PATCH] synthio: a freed track buffer, a ring modulation sign flip, reads past 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. --- shared-bindings/synthio/MidiTrack.c | 2 +- shared-bindings/synthio/Synthesizer.c | 2 +- shared-bindings/synthio/__init__.c | 6 ------ shared-module/synthio/MidiTrack.c | 4 ++++ shared-module/synthio/__init__.c | 26 ++++++++++++-------------- 5 files changed, 18 insertions(+), 22 deletions(-) diff --git a/shared-bindings/synthio/MidiTrack.c b/shared-bindings/synthio/MidiTrack.c index 0304df12a81..6adb11e06c3 100644 --- a/shared-bindings/synthio/MidiTrack.c +++ b/shared-bindings/synthio/MidiTrack.c @@ -72,7 +72,7 @@ static mp_obj_t synthio_miditrack_make_new(const mp_obj_type_t *type, size_t n_a synthio_miditrack_obj_t *self = mp_obj_malloc(synthio_miditrack_obj_t, &synthio_miditrack_type); common_hal_synthio_miditrack_construct(self, (uint8_t *)bufinfo.buf, bufinfo.len, - args[ARG_tempo].u_int, + mp_arg_validate_int_min(args[ARG_tempo].u_int, 1, MP_QSTR_tempo), args[ARG_sample_rate].u_int, args[ARG_waveform].u_obj, mp_const_none, diff --git a/shared-bindings/synthio/Synthesizer.c b/shared-bindings/synthio/Synthesizer.c index 35cc42a2037..a5d053df302 100644 --- a/shared-bindings/synthio/Synthesizer.c +++ b/shared-bindings/synthio/Synthesizer.c @@ -63,7 +63,7 @@ static mp_obj_t synthio_synthesizer_make_new(const mp_obj_type_t *type, size_t n synthio_synthesizer_obj_t *self = mp_obj_malloc(synthio_synthesizer_obj_t, &synthio_synthesizer_type); common_hal_synthio_synthesizer_construct(self, - args[ARG_sample_rate].u_int, + mp_arg_validate_int_min(args[ARG_sample_rate].u_int, 1, MP_QSTR_sample_rate), args[ARG_channel_count].u_int, args[ARG_waveform].u_obj, args[ARG_envelope].u_obj); diff --git a/shared-bindings/synthio/__init__.c b/shared-bindings/synthio/__init__.c index 6a21834751b..5ef0b58b710 100644 --- a/shared-bindings/synthio/__init__.c +++ b/shared-bindings/synthio/__init__.c @@ -258,12 +258,6 @@ static mp_obj_t synthio_from_file(size_t n_args, const mp_obj_t *pos_args, mp_ma args[ARG_envelope].u_obj ); - #if MICROPY_MALLOC_USES_ALLOCATED_SIZE - m_free(buffer, track_size); - #else - m_free(buffer); - #endif - return MP_OBJ_FROM_PTR(result); } MP_DEFINE_CONST_FUN_OBJ_KW(synthio_from_file_obj, 1, synthio_from_file); diff --git a/shared-module/synthio/MidiTrack.c b/shared-module/synthio/MidiTrack.c index 1cccd440fad..7c54c36d47f 100644 --- a/shared-module/synthio/MidiTrack.c +++ b/shared-module/synthio/MidiTrack.c @@ -19,6 +19,7 @@ static mp_obj_t parse_note(synthio_miditrack_obj_t *self) { size_t len = self->track.len; if (self->pos + 1 >= len) { record_midi_stream_error(self); + return MP_OBJ_NEW_SMALL_INT(0); } uint8_t note = buffer[(self->pos)++]; if (note > 127 || buffer[(self->pos)++] > 127) { @@ -51,6 +52,9 @@ static void decode_until_pause(synthio_miditrack_obj_t *self) { uint8_t *buffer = self->track.buf; size_t len = self->track.len; do { + if (self->pos >= len) { + break; + } switch (buffer[self->pos++] >> 4) { case 8: { // Note Off mp_obj_t note = parse_note(self); diff --git a/shared-module/synthio/__init__.c b/shared-module/synthio/__init__.c index 368bae24140..6201ad96347 100644 --- a/shared-module/synthio/__init__.c +++ b/shared-module/synthio/__init__.c @@ -224,14 +224,13 @@ static bool synth_note_into_buffer(synthio_synth_t *synth, int chan, int32_t *ou uint32_t lim = waveform_length << SYNTHIO_FREQUENCY_SHIFT; uint32_t accum = synth->accum[chan]; - if (dds_rate > lim / 2) { + if (dds_rate > (lim - offset) / 2) { // beyond nyquist, can't play note return false; } - // can happen if note waveform gets set mid-note, but the expensive modulo is usually avoided - if (accum > lim) { - accum = accum % lim + offset; + if (accum >= lim) { + accum = accum < offset ? offset : offset + (accum - offset) % (lim - offset); } // first, fill with waveform @@ -247,20 +246,19 @@ static bool synth_note_into_buffer(synthio_synth_t *synth, int chan, int32_t *ou synth->accum[chan] = accum; if (ring_dds_rate) { - if (ring_dds_rate > lim / 2) { + accum = synth->ring_accum[chan]; + offset = ring_waveform_start << SYNTHIO_FREQUENCY_SHIFT; + lim = ring_waveform_length << SYNTHIO_FREQUENCY_SHIFT; + + if (ring_dds_rate > (lim - offset) / 2) { // beyond nyquist, can't play ring (but did synth main sound so // return true) return true; } - // now modulate by ring and accumulate - accum = synth->ring_accum[chan]; - offset = ring_waveform_start << SYNTHIO_FREQUENCY_SHIFT; - lim = ring_waveform_length << SYNTHIO_FREQUENCY_SHIFT; - // can happen if note waveform gets set mid-note, but the expensive modulo is usually avoided - if (accum > lim) { - accum = accum % lim + offset; + if (accum >= lim) { + accum = accum < offset ? offset : offset + (accum - offset) % (lim - offset); } for (uint16_t i = 0; i < dur; i++) { @@ -270,8 +268,8 @@ static bool synth_note_into_buffer(synthio_synth_t *synth, int chan, int32_t *ou accum = accum - lim + offset; } int16_t idx = accum >> SYNTHIO_FREQUENCY_SHIFT; - int16_t wi = (ring_waveform[idx] * out_buffer32[i]) / 32768; // consider for synthio_sat16 but had a weird artificat - out_buffer32[i] = wi; + int32_t wi = (ring_waveform[idx] * out_buffer32[i]) / 32768; + out_buffer32[i] = wi > 32767 ? 32767 : wi; } synth->ring_accum[chan] = accum; }