Assert the nbytes range in sign_extend - #852
Open
thatssoheil wants to merge 1 commit into
Open
thatssoheil wants to merge 1 commit into
thatssoheil wants to merge 1 commit into
Conversation
sign_extend is private, and every caller reaches it through get_uint / get_uint_le / try_get_uint / try_get_uint_le, which reject more than 8 bytes before it runs, so `nbytes - 1` underflowing would mean a caller violated that contract rather than that a read came up short. Assert the range instead of subtracting blind, which is what tokio-rs#832 asked for. The assertion suggested in tokio-rs#832, `debug_assert!(nbytes >= 1 && ...)`, cannot be used as written: it panics on the existing zero-length read path, which returns 0 rather than shifting by 64, and fails 27 tests. This asserts the upper bound only and leaves that path as it was. Tests: sign_extend rejects more than eight bytes (the assertion's own panic), and a zero-length read still returns 0. Fixes tokio-rs#832
There was a problem hiding this comment.
🟢 Approval recommended
The assertion and tests cover the intended bounds without changing valid behavior.
Pull request overview
Adds a debug-only upper-bound assertion to sign_extend while preserving zero-length reads.
Changes:
- Assert
nbytes <= 8. - Add tests for oversized and zero-length inputs.
File summaries
| File | Description |
|---|---|
src/buf/buf_impl.rs |
Adds the assertion and focused unit tests. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
This adds the assertion #832 asks for, plus the two tests that pin the range it asserts.
sign_extendis private, and all four callers (get_int,get_int_le,try_get_int,try_get_int_le) reach it throughget_uint/get_uint_le/try_get_uint/try_get_uint_le, which reject more than 8 bytes viapanic_does_not_fitbeforesign_extendruns. Sonbytes > 8there means a caller broke that contract rather than that a read came up short, and the subtraction is best not done blind:One note on the assertion suggested in the issue itself,
debug_assert!(nbytes >= 1 && ...): it cannot be used as written. A zero-length read is a real input here, handled by the existingif nbytes == 0branch that returns 0 instead of shifting by 64, and assertingnbytes >= 1panics on it and fails 27 existing tests. This asserts the upper bound only and leaves that branch as it was.No release-mode behaviour changes:
debug_assertis compiled out, and the assert cannot fire on any input the public API previously accepted.Tests
cargo testpasses in debug and release, andcargo fmt --checkis clean.Two tests in a
#[cfg(test)] mod testsat the end of the file:sign_extend_asserts_more_than_eight_bytes:#[should_panic(expected = "nbytes must be in [0, 8]")]forsign_extend(0, 9), gated with#[cfg(debug_assertions)]because the assertion is compiled out of a release build.sign_extend_accepts_a_zero_length_read: pins the zero-length read at 0.Deleting the assertion makes the first fail with "panic did not contain expected string", so both exercise the change.
Disclosure
AI-assisted. I ran the checks above and can explain every line during review.
Fixes #832