tls_codec: add variable-length integer type TlsVarInt #1656
Conversation
As defined in #[rfc9000]. Also use this type (with an internal thin wrapper `ContentLength`) when encoding/deconding the content length of vectors. [rfc9000]: https://www.rfc-editor.org/rfc/rfc9000#name-variable-length-integer-enc
|
This totally fell of my radar. Sorry for that. I'll look at this soon. |
franziskuskiefer
left a comment
franziskuskiefer
left a comment
There was a problem hiding this comment.
Can you say a little more about what this PR is supposed to achieve? I struggle a little with understanding what it tries to do beyond refactoring. The variable length encoding is really only internal to the TLS encoding.
| /// Returns the number of bytes required to encode this variable-length int. | ||
| pub(crate) const fn bytes_len(&self) -> usize { | ||
| let value = self.0; | ||
| if !cfg!(fuzzing) { |
There was a problem hiding this comment.
This check doesn't make sense anymore since it's not possible anymore to create value like this.
| let (len_len_byte, mut remainder) = u8::tls_deserialize_bytes(bytes)?; | ||
| impl ContentLength { | ||
| #[cfg(not(feature = "mls"))] | ||
| #[allow(dead_code)] // used in arbitrary |
There was a problem hiding this comment.
Why is it only dead code here, but not when mls is enabled?
| /// Wraps an unsinged integer as variable-length int. | ||
| /// | ||
| /// Returns `None` if the value is larger than [`Self::MAX`]. | ||
| pub const fn new(value: u64) -> Option<Self> { |
There was a problem hiding this comment.
This should rather return a Result. Then you also don't need the try_new.
| if !cfg!(fuzzing) { | ||
| debug_assert!(len <= 8, "Invalid varint len {len}"); | ||
| } | ||
| if len > 8 { | ||
| return Err(Error::LibraryError); | ||
| } |
There was a problem hiding this comment.
This looks like it's caught by the catch all _ in the match below.
| #[inline(always)] | ||
| fn read_variable_length_bytes(bytes: &[u8]) -> Result<((usize, usize), &[u8]), Error> { | ||
| // The length is encoded in the first two bits of the first byte. | ||
| /// Thin wrapper around [`TlsVarInt`] representing the length of encoded vector content in bytes. |
There was a problem hiding this comment.
Please keep comments within 80 characters as well.
| let mut out = Vec::with_capacity(content_length + len_len); | ||
| out.append(&mut length); | ||
| out.resize(len_len, 0); | ||
| length.0.write_bytes(&mut out)?; |
There was a problem hiding this comment.
This looks like a footgun. Why not just extend the out?
| use crate::{Deserialize, Serialize}; | ||
|
|
||
| impl Deserialize for ContentLength { | ||
| fn tls_deserialize<R: std::io::Read>(bytes: &mut R) -> Result<Self, Error> { |
There was a problem hiding this comment.
Should this get inlined as well?
The idea was to expose an additional type in the codec, to allow encoding/decoding of variable length integers. This is especially useful when prefixing small TLS encoded structs with such integers. However, personally I stopped using this type, so if there no need for it for the TLS codec users, we can also close this PR. WDYT? |
3 participants
As defined in #rfc9000.
Also use this type (with an internal thin wrapper
ContentLength) whenencoding/deconding the content length of vectors.