Remove shift/overflow UB in Rice and mid/side encode; add Rice tests#35
Merged
Conversation
- Rice::encode: guard the `u >> k` / `1u << k` shifts so k >= 32 is well defined (it is unreachable for valid streams, but was undefined behaviour). Rice::decode already rejects k > 31. - ms_encode_scalar_impl: compute l+r / l-r in uint32 to avoid signed-overflow UB. This reproduces the NEON vaddq_s32/vsubq_s32 wrapping bit-for-bit and is identical to the int32 form for the validated 16/24-bit domain. - Add tests/test_rice.cpp (lac_rice_tests): round-trip across k, full-range boundary values (incl INT32_MIN -> 0xFFFFFFFF) at k == 31, and the k > 31 decode rejection. Built with -UNDEBUG and run under the ASan/UBSan CI job. Audited the other arithmetic in #8: fixed/FIR/LPC residual + restore already use int64 accumulation with int32 range checks, so they are left unchanged. Refs #8 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
What
Phase 2 of
docs/refactor-roadmap.md(#8): remove undefined behaviour in the codec hot paths and lock it in with sanitizer-backed tests.Changes
Rice::encode— guard theu >> kand1u << kshifts sok >= 32is well-defined.kis<= 31by construction (adaptivekclamps to 31; statickis au5field) andRice::decodealready rejectsk > 31, but the shifts were undefined behaviour fork >= 32.ms_encode_scalar_impl— computel + randl - rinuint32instead ofint32, removing signed-overflow UB. This reproduces the wrapping semantics of the NEONvaddq_s32/vsubq_s32path bit-for-bit, and is identical to the previousint32form for the validated 16/24-bit sample domain (which cannot overflow).tests/test_rice.cpp(newlac_rice_tests, built with-UNDEBUG) — round-trip across allk, full-range boundary values (incl.INT32_MIN→ unsigned0xFFFFFFFF) at thek == 31shift boundary, and thek > 31decode-rejection contract. Runs under the existing ASan/UBSan CI job.Audit of the remaining #8 items
decoder.cpp,main.cpp) already usesint64+ range checks.block/encoder.cpp) and restore (block/decoder.cpp,lpc.cpp) already accumulate inint64and range-check before narrowing toint32.So those paths are left unchanged. Marking Refs #8 (not Closes): a follow-up could add full-range synthetic-input tests for the predictor/restore primitives, though they are already range-checked.
Verification (local, macOS)
ctest3/3 pass (incl. newlac_rice_tests).ctest3/3 pass andlac_cli selftestclean — no sanitizer reports.Refs #8