cipher: document SeekNum encoding conventions and debug panic - #2481
Open
tautschnig wants to merge 2 commits into
Open
cipher: document SeekNum encoding conventions and debug panic#2481tautschnig wants to merge 2 commits into
tautschnig wants to merge 2 commits into
Conversation
from_block_byte and into_block_byte use different (block, byte) encodings (keystream-buffer convention with byte in 1..=bs vs. position division with byte in 0..bs) and are not inverses of each other: feeding the output of into_block_byte back into from_block_byte panics in debug builds (debug_assert!(byte != 0) fires for positions at exact block boundaries) and silently computes a wrong position in release builds (e.g. 16 -> 0 for bs=16). Only '# Errors' was documented. Document both conventions, their non-inverse relationship, and the debug panic. Found by running Kani's autoharness (model-checking/kani#3832) over cipher 0.5.2, which reported the debug_assert reachable; the wrapper's try_seek/try_current_pos use the conventions correctly, so this is a documentation gap on the public trait, not a functional bug in the wrapper. Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR improves the public documentation for SeekNum::{from_block_byte, into_block_byte} by clarifying that they intentionally use different (block, byte) encodings (and therefore are not inverses), and by documenting the debug-assert panic behavior in from_block_byte when byte == 0.
Changes:
- Document the keystream-buffer encoding expected by
SeekNum::from_block_byteand explicitly note it differs frominto_block_byte. - Document the position-division encoding returned by
SeekNum::into_block_byteand explicitly note it differs fromfrom_block_byte. - Add
# Panicsdocs describing the debug-assert panic scenario forfrom_block_byte.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
291
to
292
| /// # Errors | ||
| /// Returns [`OverflowError`] in the event of a counter overflow. |
Comment on lines
306
to
308
| /// | ||
| /// # Errors | ||
| /// Returns [`OverflowError`] in the event of a counter overflow. |
Review feedback: OverflowError is also returned for out-of-range inputs (byte > bs, unconvertible block), and into_block_byte divides by bs, so bs == 0 panics; document both. Co-authored-by: Kiro <kiro-agent@users.noreply.github.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
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.
SeekNum::from_block_byteandSeekNum::into_block_byteuse different(block, byte)encodings and are not inverses of each other:into_block_bytereturns the position-division pair:block = self / bs,byte = self % bs(sobyteis in0..bs);from_block_byteexpects the keystream-buffer pair used byStreamCipherCoreWrapper::try_current_pos:blockis the next block to generate andbytein1..=bsis the consumed byte count, computingblock * bs - (bs - byte).Feeding the output of one into the other therefore misbehaves: for a position at an exact block boundary (e.g.
16u32.into_block_byte(16)=(1, 0)),from_block_byte(1, 0, 16)panics in debug builds (debug_assert!(byte != 0)) and silently returns the wrong position (0instead of16) in release builds. Only# Errorswas documented.This PR documents both conventions, their non-inverse relationship, and the debug panic — no functional change, since
StreamCipherCoreWrapperuses the conventions consistently and correctly. If you would rather make the pair self-consistent (or replace thedebug_assertwith anOverflowError), happy to rework in that direction instead.Found by running Kani's autoharness (model-checking/kani#3832) over cipher 0.5.2, which reported the
debug_assertreachable on the public trait method; reproduced with plain cargo before filing.