tls_codec: add variable-length integer type TlsVarInt by boxdot · Pull Request #1656 · RustCrypto/formats

boxdot · 2025-02-13T10:08:46Z

As defined in #rfc9000.

Also use this type (with an internal thin wrapper ContentLength) when
encoding/deconding the content length of vectors.

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

franziskuskiefer · 2025-07-14T10:12:30Z

This totally fell of my radar. Sorry for that. I'll look at this soon.

franziskuskiefer

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.

franziskuskiefer · 2026-02-06T09:56:32Z

tls_codec/src/varint.rs

+    /// 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) {


This check doesn't make sense anymore since it's not possible anymore to create value like this.

Makes sense. Removed.

franziskuskiefer · 2026-02-06T10:00:33Z

tls_codec/src/quic_vec.rs

-    let (len_len_byte, mut remainder) = u8::tls_deserialize_bytes(bytes)?;
+impl ContentLength {
+    #[cfg(not(feature = "mls"))]
+    #[allow(dead_code)] // used in arbitrary


Why is it only dead code here, but not when mls is enabled?

I also put it behind the arbitrary feature which makes dead_code unnecessary.

franziskuskiefer · 2026-02-06T10:00:53Z

tls_codec/src/varint.rs

+    /// 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> {


This should rather return a Result. Then you also don't need the try_new.

franziskuskiefer · 2026-02-06T10:03:04Z

tls_codec/src/varint.rs

+        if !cfg!(fuzzing) {
+            debug_assert!(len <= 8, "Invalid varint len {len}");
+        }
+        if len > 8 {
+            return Err(Error::LibraryError);
+        }


This looks like it's caught by the catch all _ in the match below.

Removed this check and changed the error variant in the catch all case to library error.

franziskuskiefer · 2026-02-06T10:09:23Z

tls_codec/src/quic_vec.rs

-#[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.


Please keep comments within 80 characters as well.

Reformatted comments here and below.

franziskuskiefer · 2026-02-06T10:14:56Z

tls_codec/src/quic_vec.rs

        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)?;


This looks like a footgun. Why not just extend the out?

This is a performance optimization to avoid constructing a temporary buffer which is expensive espcially in hot loops.

There is no buffer which contains the bytes of length.0: TlsVarInt. Before, the code was constructing a buffer with write_variable_length.

franziskuskiefer · 2026-02-06T10:17:07Z

tls_codec/src/quic_vec.rs

    use crate::{Deserialize, Serialize};

+    impl Deserialize for ContentLength {
+        fn tls_deserialize<R: std::io::Read>(bytes: &mut R) -> Result<Self, Error> {


Should this get inlined as well?

boxdot · 2026-02-09T11:05:00Z

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.

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?

franziskuskiefer · 2026-02-12T11:39:06Z

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?

Thanks for clarifying.
I think there's some nice refactoring in here. I just wasn't sure about the purpose of making it public.
But encapsulating the variable length encoding looks much nicer.

franziskuskiefer · 2026-02-16T07:43:11Z

@boxdot do you want to address the comments? I'd be happy to get this in then (with public or non-public varint, if it's something that's useful sometimes, there's no harm in exposing it).

boxdot · 2026-02-17T08:01:51Z

tls_codec/src/quic_vec.rs

-    let (len_len_byte, mut remainder) = u8::tls_deserialize_bytes(bytes)?;
+impl ContentLength {
+    #[cfg(not(feature = "mls"))]
+    #[allow(dead_code)] // used in arbitrary


I also put it behind the arbitrary feature which makes dead_code unnecessary.

boxdot · 2026-02-17T08:02:00Z

tls_codec/src/quic_vec.rs

-#[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.


Reformatted comments here and below.

boxdot · 2026-02-17T08:09:39Z

tls_codec/src/quic_vec.rs

        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)?;


This is a performance optimization to avoid constructing a temporary buffer which is expensive espcially in hot loops.

There is no buffer which contains the bytes of length.0: TlsVarInt. Before, the code was constructing a buffer with write_variable_length.

boxdot · 2026-02-17T08:13:46Z

tls_codec/src/varint.rs

+    /// 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> {


boxdot · 2026-02-17T08:14:58Z

tls_codec/src/varint.rs

+    /// 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) {


Makes sense. Removed.

boxdot · 2026-02-17T08:22:32Z

tls_codec/src/varint.rs

+        if !cfg!(fuzzing) {
+            debug_assert!(len <= 8, "Invalid varint len {len}");
+        }
+        if len > 8 {
+            return Err(Error::LibraryError);
+        }


Removed this check and changed the error variant in the catch all case to library error.

boxdot · 2026-02-17T08:23:00Z

tls_codec/src/varint.rs

+    }
+}
+
+#[inline(always)]


Removed this inline, because it is a private function and it will be inlined anyways.

boxdot · 2026-02-17T08:25:14Z

@boxdot do you want to address the comments? I'd be happy to get this in then (with public or non-public varint, if it's something that's useful sometimes, there's no harm in exposing it).

I addressed review comments. We can do another review round.

tls_codec: add variable-length integer type TlsVarInt

9846c69

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

boxdot force-pushed the varint branch from 3227a54 to 9846c69 Compare February 13, 2025 10:44

boxdot added 4 commits February 13, 2025 11:49

remove std feature from DeserializeBytes impl for TlsVarInt

17b19b6

split tests in std and non-std parts

8ff32d6

non minimum-size encoding should panic when MLS flag is enabled

2e7e024

better test name

9996251

franziskuskiefer self-requested a review February 17, 2025 08:35

kkohbrok approved these changes Feb 5, 2026

View reviewed changes

franziskuskiefer reviewed Feb 6, 2026

View reviewed changes

boxdot added 3 commits February 17, 2026 08:53

Merge remote-tracking branch 'origin/master' into varint

fa6b83d

wrap comments at 80 chars

0bd1576

address review comments

d80d228

boxdot commented Feb 17, 2026

View reviewed changes

fix typo

e512c49

franziskuskiefer self-requested a review February 17, 2026 12:57

Conversation

boxdot commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

franziskuskiefer commented Jul 14, 2025

Uh oh!

franziskuskiefer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boxdot commented Feb 9, 2026

Uh oh!

franziskuskiefer commented Feb 12, 2026

Uh oh!

franziskuskiefer commented Feb 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boxdot commented Feb 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

boxdot commented Feb 13, 2025 •

edited

Loading