Skip to content

Bound selected frame duration before multiplication - #513

Closed
nailongzhanshen666 wants to merge 1 commit into
xiph:mainfrom
nailongzhanshen666:fix/491-frame-size-validation
Closed

nailongzhanshen666 wants to merge 1 commit into
xiph:mainfrom
nailongzhanshen666:fix/491-frame-size-validation

Conversation

@nailongzhanshen666

Copy link
Copy Markdown

Summary

Closes #491.

Reject a selected frame longer than 120 ms before the duration checks multiply new_size. The limit is computed as 3*(Fs/25), keeping the arithmetic within range for the supported sample rates without introducing 64-bit multiplications. Apply the limit after the expert-duration selection, so selecting a short frame from a longer input remains valid.

Add a direct frame_size_select() regression in test_opus_encode, before the existing encoder regressions. It covers the reported value, the nine legal durations at six sample rates, the upper boundary, INT_MAX, and forced-duration selection. Calling the selection helper directly avoids allocating an enormous PCM buffer or provoking the downstream stack allocation just to test argument validation.

Verification

GCC 13.3.0, CMake 3.28.3, Linux x86-64, Release builds with:

-fsanitize=signed-integer-overflow -fno-sanitize-recover=signed-integer-overflow
  • On the unmodified bafd40dcbd506e12da257b8cb05c0afe1ed44719 implementation, the new regression exits with runtime error: signed integer overflow: 1073741864 * 400 cannot be represented in type 'int'.
  • After the fix, test_opus_api and test_opus_encode pass in both the default floating-point build and OPUS_FIXED_POINT=ON (2/2 CTest targets in each build).
  • Runs use SEED=1 TEST_OPUS_NOFUZZ=1; the existing regressions and encoding tests run, while the optional garbage-decoder/online-setting fuzzing is skipped.
  • git diff --check passes; the tested builds emitted no compiler warnings.

This is not a claim of full ASan/UBSan, all-target CTest, hardware, or fuzz-campaign coverage. The observed local failure is the signed overflow in validation, not a reproduction of the report's downstream stack crash.

Credits and earlier attempts

Thanks to @Amelia-Wang-Hanyu/FuzzAnything for the public report. @afonsojanu previously proposed fixes in #494 and #500; both were closed by their author during general PR cleanup on 2026-10-05. This is a follow-up proposal with a selected-duration bound and direct selection regression, not a new vulnerability discovery. No open replacement PR was found when submitting this change.

AI assistance was used for the code, tests, local validation, and this description.

@jmvalin

jmvalin commented Oct 31, 2026

Copy link
Copy Markdown
Member

Thanks for reviving the issue. I chose to land #494 instead (similar change, but test didn't break shared library build)

@jmvalin jmvalin closed this Oct 31, 2026
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.

Signed integer overflow in frame_size_select's own validation lets a huge frame_size through to a stack allocation

2 participants