Fix signed-integer-overflow UB in interpolate()'s FIR accumulator - #68
Open
austek wants to merge 1 commit into
Open
Fix signed-integer-overflow UB in interpolate()'s FIR accumulator#68austek 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, confirmed via a UBSan trace during fuzzing: signed integer overflow at sonic.c:956. Replaced with accumulation in long (matching this file's existing convention -- insertPitchPeriod already widens to long for the same reason), 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 fixed elsewhere in this file (findSincCoefficient, tracked separately as waywardgeek#50). Fixes waywardgeek#66. 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 during development to trip the old, unfixed code and to no longer do so after the fix, while still returning the same correctly-clamped SHRT_MAX. Note: findSincCoefficient's separate, already-tracked left-shift bug (waywardgeek#50, PR waywardgeek#62) sits on the same call path (interpolate calls it every invocation) and is NOT fixed here, kept deliberately out of scope to avoid duplicating that PR's diff. Verified this fix is complete and correct in isolation by additionally patching that unrelated bug in a scratch copy and confirming the full test suite is then completely clean under ASan+UBSan.
2 tasks
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.
Fixes #66.
interpolate()accumulates 12 sinc-filter tap products into anint total, detecting overflow via a sign-flip heuristic (oldSign != getSign(total)) that itself relies on the signed overflow having already happened — undefined behavior in C, confirmed via a UBSan trace found while fuzzing:signed integer overflowatsonic.c:956.Fix: accumulate in
longinstead (matching this file's existing convention —insertPitchPeriodalready widens tolongfor the same reason), 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.getSignhad no other callers and is removed. The clamp bounds are computed via multiplication ((long)SHRT_MIN * 65536L), not left-shift —SHRT_MINis negative, and left-shifting a negative value is undefined behavior, the same class of bug present elsewhere in this file (findSincCoefficient).Adds
tests/interpolate_overflow_test.c— a white-box test callinginterpolate()directly (notstatic, for this reason). On a freshly created streamnewRatePosition/oldRatePositionare both0, which makes the internalratio/widthdepend only onnewSampleRate, so each tap's weight is known in advance; setting each sample to the maximum magnitude matching its tap's coefficient sign guarantees the true sum exceedsINT_MAX. Verified via UBSan during development: trips the old code, doesn't trip the fixed code, and both return the same correctly-clampedSHRT_MAX.Deliberately not included:
findSincCoefficient's separate left-shift bug sits on the exact same call path (interpolatecalls it every invocation) but isn't fixed here, to keep this diff focused — it's fixed instead as a drive-by in the CI PR (#67), which needs it for thestrict-and-sanitizedjob to pass-Werror+UBSan. Verified this fix is complete and correct in isolation by additionally patching that unrelated bug in a scratch copy and confirming the whole test suite is then completely clean under ASan+UBSan — once both this PR and #67 land,masterwill be fully clean.