From b34aa34c19c8d1881ba1559342fb0b5415363ce5 Mon Sep 17 00:00:00 2001 From: Peter Bay Date: Sun, 13 Sep 2026 21:25:48 +0200 Subject: [PATCH] audio: validate arguments before narrowing them, and end an MP3 with DONE audiomixer narrowed the voice index into a uint8_t before comparing it with voice_count, so voice=256 became 0 and played on the first voice. PDMIn did the same with bit_depth and oversample, ahead of the divisibility check that 256 then passed as zero, and never checked sample_rate. The sample_rate setter shared by every audiosample validated nothing at all, so it could undo the constructors' own check. I2SOut's left_justified was declared MP_ARG_OBJ and read as .u_bool, so passing it explicitly gave whatever the object pointer aliased to. audiomixer's mono path stepped two samples at a time and wrote word_buffer[i] and word_buffer[i + 1], one past the end for an odd n. An input underflow at the end of the data is how an MP3 cut off mid-frame ends, not a failure, but get_buffer returned GET_BUFFER_ERROR for it and a looped playback stopped instead of restarting. It returns GET_BUFFER_DONE for an underflow at end of file now. stream_lseek was also given its offset and whence the wrong way round in two places, and the success test compared against zero where a stream that cannot seek returns its unchanged position. --- shared-bindings/audiobusio/I2SOut.c | 2 +- shared-bindings/audiobusio/PDMIn.c | 12 +++++++----- shared-bindings/audiocore/__init__.c | 3 ++- shared-bindings/audiomixer/Mixer.c | 12 ++++-------- shared-module/audiomixer/Mixer.c | 2 +- shared-module/audiomp3/MP3Decoder.c | 8 +++++--- 6 files changed, 20 insertions(+), 19 deletions(-) diff --git a/shared-bindings/audiobusio/I2SOut.c b/shared-bindings/audiobusio/I2SOut.c index a104b20b9da..7769352a1da 100644 --- a/shared-bindings/audiobusio/I2SOut.c +++ b/shared-bindings/audiobusio/I2SOut.c @@ -98,7 +98,7 @@ static mp_obj_t audiobusio_i2sout_make_new(const mp_obj_type_t *type, size_t n_a { MP_QSTR_word_select, MP_ARG_OBJ | MP_ARG_REQUIRED }, { MP_QSTR_data, MP_ARG_OBJ | MP_ARG_REQUIRED }, { MP_QSTR_main_clock, MP_ARG_OBJ | MP_ARG_KW_ONLY, {.u_obj = mp_const_none} }, - { MP_QSTR_left_justified, MP_ARG_OBJ | MP_ARG_KW_ONLY, {.u_bool = false} }, + { MP_QSTR_left_justified, MP_ARG_BOOL | MP_ARG_KW_ONLY, {.u_bool = false} }, { MP_QSTR_external_clock, MP_ARG_BOOL | MP_ARG_KW_ONLY, {.u_bool = false} }, }; mp_arg_val_t args[MP_ARRAY_SIZE(allowed_args)]; diff --git a/shared-bindings/audiobusio/PDMIn.c b/shared-bindings/audiobusio/PDMIn.c index aba8a019f35..075c089841c 100644 --- a/shared-bindings/audiobusio/PDMIn.c +++ b/shared-bindings/audiobusio/PDMIn.c @@ -96,15 +96,17 @@ static mp_obj_t audiobusio_pdmin_make_new(const mp_obj_type_t *type, size_t n_ar const mcu_pin_obj_t *clock_pin = validate_obj_is_free_pin(args[ARG_clock_pin].u_obj, MP_QSTR_clock_pin); const mcu_pin_obj_t *data_pin = validate_obj_is_free_pin(args[ARG_data_pin].u_obj, MP_QSTR_data_pin); - uint32_t sample_rate = args[ARG_sample_rate].u_int; - uint8_t bit_depth = args[ARG_bit_depth].u_int; - if (bit_depth % 8 != 0) { + uint32_t sample_rate = mp_arg_validate_int_min(args[ARG_sample_rate].u_int, 1, MP_QSTR_sample_rate); + mp_int_t requested_bit_depth = args[ARG_bit_depth].u_int; + if (requested_bit_depth != 8 && requested_bit_depth != 16) { mp_raise_ValueError_varg(MP_ERROR_TEXT("%q must be multiple of 8."), MP_QSTR_bit_depth); } - uint8_t oversample = args[ARG_oversample].u_int; - if (oversample % 8 != 0) { + uint8_t bit_depth = requested_bit_depth; + mp_int_t requested_oversample = args[ARG_oversample].u_int; + if (requested_oversample < 8 || requested_oversample > 255 || requested_oversample % 8 != 0) { mp_raise_ValueError_varg(MP_ERROR_TEXT("%q must be multiple of 8."), MP_QSTR_oversample); } + uint8_t oversample = requested_oversample; bool mono = args[ARG_mono].u_bool; mp_float_t startup_delay = (args[ARG_startup_delay].u_obj == MP_OBJ_NULL) diff --git a/shared-bindings/audiocore/__init__.c b/shared-bindings/audiocore/__init__.c index b2b6c4da0f3..2dadc8877f9 100644 --- a/shared-bindings/audiocore/__init__.c +++ b/shared-bindings/audiocore/__init__.c @@ -145,7 +145,8 @@ MP_DEFINE_CONST_FUN_OBJ_1(audiosample_get_sample_rate_obj, audiosample_obj_get_s static mp_obj_t audiosample_obj_set_sample_rate(mp_obj_t self_in, mp_obj_t sample_rate) { audiosample_base_t *self = MP_OBJ_TO_PTR(self_in); audiosample_check_for_deinit(self); - audiosample_set_sample_rate(audiosample_check(self_in), mp_obj_get_int(sample_rate)); + audiosample_set_sample_rate(audiosample_check(self_in), + (uint32_t)mp_arg_validate_int_min(mp_obj_get_int(sample_rate), 1, MP_QSTR_sample_rate)); return mp_const_none; } MP_DEFINE_CONST_FUN_OBJ_2(audiosample_set_sample_rate_obj, audiosample_obj_set_sample_rate); diff --git a/shared-bindings/audiomixer/Mixer.c b/shared-bindings/audiomixer/Mixer.c index 516d079cd2a..89f0e3479da 100644 --- a/shared-bindings/audiomixer/Mixer.c +++ b/shared-bindings/audiomixer/Mixer.c @@ -185,10 +185,8 @@ static mp_obj_t audiomixer_mixer_obj_play(size_t n_args, const mp_obj_t *pos_arg mp_arg_val_t args[MP_ARRAY_SIZE(allowed_args)]; mp_arg_parse_all(n_args - 1, pos_args + 1, kw_args, MP_ARRAY_SIZE(allowed_args), allowed_args, args); - uint8_t v = args[ARG_voice].u_int; - if (v > (self->voice_count - 1)) { - mp_arg_error_invalid(MP_QSTR_voice); - } + uint8_t v = mp_arg_validate_int_range(args[ARG_voice].u_int, 0, self->voice_count - 1, + MP_QSTR_voice); audiomixer_mixervoice_obj_t *voice = MP_OBJ_TO_PTR(self->voice[v]); mp_obj_t sample = args[ARG_sample].u_obj; common_hal_audiomixer_mixervoice_play(voice, sample, args[ARG_loop].u_bool); @@ -212,10 +210,8 @@ static mp_obj_t audiomixer_mixer_obj_stop_voice(size_t n_args, const mp_obj_t *p mp_arg_val_t args[MP_ARRAY_SIZE(allowed_args)]; mp_arg_parse_all(n_args - 1, pos_args + 1, kw_args, MP_ARRAY_SIZE(allowed_args), allowed_args, args); - uint8_t v = args[ARG_voice].u_int; - if (v > (self->voice_count - 1)) { - mp_arg_error_invalid(MP_QSTR_voice); - } + uint8_t v = mp_arg_validate_int_range(args[ARG_voice].u_int, 0, self->voice_count - 1, + MP_QSTR_voice); audiomixer_mixervoice_obj_t *voice = MP_OBJ_TO_PTR(self->voice[v]); common_hal_audiomixer_mixervoice_stop(voice); return mp_const_none; diff --git a/shared-module/audiomixer/Mixer.c b/shared-module/audiomixer/Mixer.c index a6c4397e8bf..464117fae8d 100644 --- a/shared-module/audiomixer/Mixer.c +++ b/shared-module/audiomixer/Mixer.c @@ -287,7 +287,7 @@ static void mix_down_one_voice(audiomixer_mixer_obj_t *self, word_buffer[i] = mult16signed(word, active_lo_level, active_hi_level); } } else { - for (uint32_t i = 0; i < n; i += 2) { + for (uint32_t i = 0; i + 1 < n; i += 2) { uint32_t word = src[i >> 1]; uint32_t word_lsb = copy16lsb(word); assignmul(word_lsb, &last_word, &active_lo_level, &active_hi_level, pending_lo_level, pending_hi_level); diff --git a/shared-module/audiomp3/MP3Decoder.c b/shared-module/audiomp3/MP3Decoder.c index 463fccf9708..04dd8d2d75c 100644 --- a/shared-module/audiomp3/MP3Decoder.c +++ b/shared-module/audiomp3/MP3Decoder.c @@ -244,7 +244,8 @@ static void mp3file_skip_id3v2(audiomp3_mp3file_obj_t *self, bool block_ok) { size -= to_consume; // Next, seek in the file after the header - if (stream_lseek(self->stream, SEEK_CUR, size) == 0) { + off_t before = stream_lseek(self->stream, 0, SEEK_CUR); + if (before >= 0 && stream_lseek(self->stream, size, SEEK_CUR) == before + size) { return; } @@ -424,7 +425,7 @@ void audiomp3_mp3file_reset_buffer(audiomp3_mp3file_obj_t *self, // We don't reset the buffer index in case we're looping and we have an odd number of buffer // loads background_callback_prevent(); - if (self->eof && stream_lseek(self->stream, SEEK_SET, 0) == 0) { + if (self->eof && stream_lseek(self->stream, 0, SEEK_SET) == 0) { INPUT_BUFFER_CLEAR(self->inbuf); self->eof = 0; self->samples_decoded = 0; @@ -491,8 +492,9 @@ audioio_get_buffer_result_t audiomp3_mp3file_get_buffer(audiomp3_mp3file_obj_t * if (self->eof || (err != ERR_MP3_INDATA_UNDERFLOW && err != ERR_MP3_MAINDATA_UNDERFLOW)) { memset(buffer, 0, self->base.max_buffer_length); *buffer_length = 0; + bool underflow = (err == ERR_MP3_INDATA_UNDERFLOW || err == ERR_MP3_MAINDATA_UNDERFLOW); self->eof = true; - return GET_BUFFER_ERROR; + return underflow ? GET_BUFFER_DONE : GET_BUFFER_ERROR; } }