Fix signed-integer-overflow UB in interpolate()'s FIR accumulator - #3
Open
austek wants to merge 1 commit into
Open
Fix signed-integer-overflow UB in interpolate()'s FIR accumulator#3austek wants to merge 1 commit into
austek wants to merge 1 commit into
Conversation
interpolate() accumulated 12 sinc-filter tap products into an int total, detecting overflow via a sign-flip heuristic (oldSign != getSign(total)) that itself relies on the signed overflow already having happened -- undefined behavior in C. Replaced with accumulation in long, wide enough to hold the true sum without overflowing, so the final clamp compares against real bounds instead of inferring overflow from a sign flip. getSign is now unused and removed -- it had no other callers. The overflow bounds are computed via multiplication ((long)SHRT_MIN * 65536L), not left-shift: SHRT_MIN is negative, and left-shifting a negative value is undefined behavior in C, the same class of bug already present elsewhere in this file (findSincCoefficient) but not fixed here -- kept deliberately out of scope to avoid overlapping that separate fix. Adds tests/interpolate_overflow_test.c, a white-box test that calls interpolate() directly (not static, for this reason) with a deterministically constructed 12-sample input: on a freshly created stream, newRatePosition/oldRatePosition are both 0, which makes the internal ratio/width depend only on newSampleRate, so the weight at each tap is known in advance. Setting each sample to the maximum magnitude matching its tap's coefficient sign guarantees the accumulator's true sum exceeds INT_MAX -- verified via UBSan to trip the old, unfixed code and to no longer do so after the fix, while still returning the same correctly-clamped SHRT_MAX. Ported from waywardgeek/sonic#68, which fixes the same bug (also present here, byte-identical interpolate()/getSign) upstream.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
interpolate()'s 12-tap sinc-filter accumulator detected overflow with a sign-flip heuristic (oldSign != getSign(total)) that itself depends on the signed overflow already having occurred — undefined behavior in C.Replaces the
int totalaccumulator withlong, wide enough to hold the true sum, so the final clamp compares against real bounds ((long)SHRT_MAX * 65536L) instead of inferring overflow from a sign flip.getSignis now unused and removed.Same bug, same fix as waywardgeek/sonic#68 — this repo's
interpolate()/getSignare byte-identical to what that file had upstream, so the fix ports directly.Scope note
findSincCoefficient's separate left-shift-of-negative-value UB (same call path, still present in this file) is not touched here, to keep this diff focused — it's fixed instead as a drive-by in the CI PR (#6), which needs it for thestrict-and-sanitizedjob to pass-Werror+UBSan. Verified via ASan+UBSan that this fix alone removes the accumulator overflow; the only remaining sanitizer finding is that separate issue.Test plan
cd tests && make runtests && ./runtests— passes, newtests/interpolate_overflow_test.cincluded-fsanitize=address,undefined— no accumulator-overflow finding; only the separate, out-of-scopefindSincCoefficientfinding remains