fix(buffers): correct offset file id calculation in disk_v2 ledger#25539
Open
xfocus3 wants to merge 1 commit into
Open
fix(buffers): correct offset file id calculation in disk_v2 ledger#25539xfocus3 wants to merge 1 commit into
xfocus3 wants to merge 1 commit into
Conversation
`get_offset_reader_file_id` computed `current.wrapping_add(offset) % MAX_FILE_ID` in `u16`. When `MAX_FILE_ID` is `u16::MAX`, `current + offset` could wrap at `u16::MAX + 1` before the modulo was applied, so distinct offsets near the boundary collapsed to the same file id (e.g. current=65534 returned 0 for both offset=1 and offset=2). Perform the arithmetic in `u32` so the wrap happens at `MAX_FILE_ID` as intended. Closes vectordotdev#25440
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.
Summary
Closes #25440.
get_offset_reader_file_idin thedisk_v2ledger computed the offset file id withu16arithmetic:In production
MAX_FILE_ID == u16::MAX(65535). Whencurrent + offsetexceedsu16::MAX, thewrapping_addwraps at 65536 before the modulo by 65535, so distinct offsets near the boundary resolve to the same file id:This is also a likely root cause of #19759.
Fix
Perform the addition in
u32so the wrap occurs atMAX_FILE_IDas intended, then convert back tou16. The arithmetic is factored into a small helper (offset_file_id/offset_file_id_with_max) so it can be unit-tested directly (theArchivedLedgerStatemethod is otherwise hard to construct in a test).How did you test this PR?
ledger.rs:offset_file_id_wraps_at_max— basic and wrap-around behavior.offset_file_id_does_not_wrap_u16_before_modulo— exercises theu16::MAXboundary that triggered the bug (the test-onlyMAX_FILE_ID = 6can't reach it, so the helper is tested withu16::MAXexplicitly).cargo test -p vector-buffers ledger→ 4 passed.cargo clippy -p vector-buffers --all-targets -- -D warningsandcargo fmt --checkpass.