Skip to content

android: reject a zero sample rate or channel count in oto_oboe_Play - #303

Open
kumagi wants to merge 1 commit into
ebitengine:mainfrom
kumagi:fix-android-oboe-zero-channel-divide
Open

kumagi wants to merge 1 commit into
ebitengine:mainfrom
kumagi:fix-android-oboe-zero-channel-divide

Conversation

@kumagi

@kumagi kumagi commented Sep 27, 2026

Copy link
Copy Markdown
Contributor

What issue is this addressing?

No issue filed yet. Found during an audit of the Android binding.

What type of issue is this addressing?

bug

What this PR does | solves

Stream::Play (internal/oboe/binding_android.cpp) stores channel_num_ and sample_rate_ without validation, and later code divides by them:

// OpenLocked / PrepareBuffersLocked
buffer_size_in_bytes_ / channel_num_ / 4
num_frames * 1000000 / sample_rate_

If the caller ever passes 0 (e.g. a NewContextOptions with SampleRate/ChannelCount left at their zero value reaching the binding unchecked), this faults with SIGFPE on the audio thread instead of returning a configuration error. Note the Go side does not currently reject zero values either, so the binding is the last line of defense.

The fix

Validate both arguments up front and return a descriptive error string, matching the existing const char * error contract of oto_oboe_Play:

if (sample_rate <= 0) {
    return "oto_oboe_Play: sample rate must be positive";
}
if (channel_num <= 0) {
    return "oto_oboe_Play: channel count must be positive";
}

The error propagates back to newContext in driver_android.go and is reported through c.err.Join as usual.

OpenLocked divides the requested buffer size by channel_num_ and
PrepareBuffersLocked converts the buffer to microseconds by dividing by
sample_rate_, so a zero from the caller faults the process instead of
failing the way every other bad configuration does. oto does not check
the two options when the default buffer size is used, and the extern
entry point is callable on its own.
@hajimehoshi

Copy link
Copy Markdown
Member

Could we put this validation in the common NewContext entry point, before setting contextCreated, so it applies to all platforms and returns an error synchronously?

There is already a sampleRate <= 0 check in durationToBufferSize, but its early return for duration == 0 bypasses that check when the default buffer size is used. NewContext should unconditionally reject SampleRate <= 0 and validate ChannelCount as 1 or 2, matching its documented contract.

This would also avoid sending these configuration errors through Android's asynchronous initialization error path, which currently records the error without closing ready, leaving callers waiting on that channel indefinitely.

Posted by Codex on behalf of @hajimehoshi.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants