ChaCha: fix behavior on block count wrap #980
Conversation
Documented behavior is to allow the counter to wrap around, but implementation was panicking on that event in debug-mode builds. Also fix `get_word_pos` handling of counter-wrapping, add some tests, and make math easier to read.
| let mut block = u128::from(self.rng.core.state.get_stream_param(STREAM_PARAM_BLOCK)); | ||
| // counter is incremented *after* filling buffer | ||
| block -= 4; | ||
| (block << 4) + self.rng.index() as u128 | ||
| let buf_start_block = { | ||
| let buf_end_block = self.rng.core.state.get_stream_param(STREAM_PARAM_BLOCK); | ||
| u64::wrapping_sub(buf_end_block, BUF_BLOCKS.into()) | ||
| }; | ||
| let (buf_offset_blocks, block_offset_words) = { | ||
| let buf_offset_words = self.rng.index() as u64; | ||
| let blocks_part = buf_offset_words / u64::from(BLOCK_WORDS); | ||
| let words_part = buf_offset_words % u64::from(BLOCK_WORDS); | ||
| (blocks_part, words_part) | ||
| }; | ||
| let pos_block = u64::wrapping_add(buf_start_block, buf_offset_blocks); | ||
| let pos_block_words = u128::from(pos_block) * u128::from(BLOCK_WORDS); | ||
| pos_block_words + u128::from(block_offset_words) |
There was a problem hiding this comment.
Yes, so I missed that the block size is still 16 words when we're dealing with four blocks at once. But you call this easier to read? I see no advantage in splitting the buffer offset into block and word parts, or using inner code blocks here.
In general easy to comprehend implies both sufficient detail and concise.
There was a problem hiding this comment.
I see no advantage in splitting the buffer offset into block and word parts,
There are two ways to fix this function's behavior in the counter-wrapping case: use wrapping 64-bit arithmetic for the high bits, or masked 128-bit arithmetic. I could change it to the other way if you prefer.
There was a problem hiding this comment.
If I understand correctly, buf_start_block + buf_offset_blocks cannot wrap since the former should be a multiple of 4 and the latter less than 4.
There was a problem hiding this comment.
The buffer is a 4-block window. It is always at a block-aligned position in the stream but if the stream has been seeked it may not be self-aligned. The new tests fail without changing this.
| // NB. this must remain consistent with some currently hard-coded numbers in this module | ||
| const BUF_BLOCKS: u8 = 4; | ||
| // number of 32-bit words per ChaCha block (fixed by algorithm definition) | ||
| const BLOCK_WORDS: u8 = 16; |
There was a problem hiding this comment.
These are used as u64 / u128 and should be inlined anyway, so use of u8 here just adds noise.
There was a problem hiding this comment.
I was thinking this module might be further improved in the future to use values based on the named constants rather than hard-coded numbers for its usize values. u8 is the only type that can be converted to both u64 and usize with the total functions.
Documented behavior is to allow the counter to wrap around, but
implementation was panicking on that event in debug-mode builds.
Also fix
get_word_poshandling of counter-wrapping, add some tests,and make math easier to read.