GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — audio: validate arguments before narrowing them, and end an MP3 with … by peterbay · Pull Request #11368 · adafruit/circuitpython

Conversation

@peterbay

@peterbay peterbay commented Sep 13, 2026

Copy link
Copy Markdown

Code written by Claude Code, guided and corrected by @peterbay.

The problem

Five defects across the audio modules. Four are arguments narrowed into a uint8_t or uint32_t field before anything checked them, so a value past the field's range wrapped into a valid-looking one. The fifth stops a looped MP3 from restarting.

The changes

  • audiomp3 returned an error where a stream had simply ended. An input underflow at the end of the data is how an MP3 that was cut off mid-frame finishes, not a failure, but get_buffer returned GET_BUFFER_ERROR for it and the caller stopped playback instead of looping. It returns GET_BUFFER_DONE when the underflow is at end of file, and GET_BUFFER_ERROR for a real decode error as before.

  • audiomp3 passed stream_lseek its offset and whence the wrong way round, in two places. The signature is (stream, offset, whence). The seek past an ID3 header asked to move SEEK_CUR bytes from whence size, failed, and fell through to the read-and-discard loop below, which is why it still worked. The rewind on loop passed (SEEK_SET, 0), which worked only because SEEK_SET is 0. The success test on the first one is also wrong: a stream that cannot seek returns its unchanged position, so comparing against zero is not enough. It now compares the position before and after.

  • audiomixer narrowed the voice index before checking it. args[ARG_voice].u_int went into a uint8_t and was then compared against voice_count - 1, so voice=256 became 0 and played on the first voice instead of raising. Changed to mp_arg_validate_int_range on the value as parsed.

  • audiomixer wrote one word past the mix buffer. The mono path stepped for (i = 0; i < n; i += 2) and wrote word_buffer[i] and word_buffer[i + 1], so an odd n wrote one past the end. The loop now bounds i + 1.

  • audiobusio.PDMIn narrowed bit_depth and oversample before the divisibility check, so 256 passed it as zero, and sample_rate was not checked at all. All three are validated on the parsed value now.

  • audiocore's shared sample_rate setter validated nothing. It is mixed into every audio sample through AUDIOSAMPLE_FIELDS, so it could undo the constructors' own check; zero reached code that divides by it.

  • I2SOut.left_justified was declared MP_ARG_OBJ and read as .u_bool. Passing it explicitly stored an object in the union and the flag came back as whatever that pointer aliased to, so left_justified=False read as true. Declared MP_ARG_BOOL.

Testing

Seeed XIAO nRF52840 Sense with an Adafruit Audio BFF, on two builds differing only by these changes. Playback ran through a mixer voice held at level 0, so the decoder was driven without anything audible.

before after
mixer.play(sample, voice=256) on a 2-voice mixer accepted, voice 0 plays ValueError: voice must be 0-1
sample.sample_rate = 0 accepted, reads back 0 ValueError: sample_rate must be >= 1
PDMIn(..., oversample=256) accepted ValueError: oversample must be multiple of 8.
an MP3 cut off mid-frame, played with loop=True stops after 0.8 s still looping after 6 s

PDMIn's bit_depth=256 and sample_rate=0 are rejected on this port either way, because its HAL supports only bit_depth=16 and sample_rate=16000 and refuses them for its own reason; oversample is the one it leaves to the binding. A complete MP3 never reaches the changed branch — it ends on a whole frame — so the loop test used a truncated copy.

Two of these are not covered above. The mixer's one-word overrun needs an odd n, which the buffer sizes here do not produce. left_justified sets NRF_I2S->CONFIG.FORMAT on this port, but memorymap here only maps RAM, so the register could not be read back from Python.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@peterbay
audio: validate arguments before narrowing them, and end an MP3 with …
b34aa34
…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.
dhalbert
dhalbert approved these changes Sep 13, 2026
View reviewed changes

@dhalbert dhalbert left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - these are nice fixes, and they all make sense to me.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@peterbay

peterbay commented Sep 13, 2026

Copy link
Copy Markdown
Author

Testing and diagnostic script.
audio_validate_before_narrowing.py

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dhalbert
dhalbert merged commit 913d573 into adafruit:main Sep 14, 2026
586 of 587 checks passed

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dhalbert dhalbert dhalbert approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@peterbay @dhalbert
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.