hyperium / hyperium/h2

Hyper test would fail if an existing bug in tokio-util's `LengthDelimitedCodec` is fixed

Open
#624 0 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
1.6k
Forks
382
Avg merge
20h 23m
Merged PRs (30d)
9

Description

Description

The current LengthDelimitedCodec implementation has a bug (tokio-rs/tokio#4815) where length adjustment is not applied before checking overflow. And the PR that tries to fix the bug (tokio-rs/tokio#4816) currently has failed h2_connect_large_body test with hyper because of the way LengthDelimitedCodec is used in h2 right now.

Suspected cause

The cause of the problem is that FramedRead internally uses a LengthDelimitedCodec while FramedWrite uses a custom encoder. Though the LengthDelimitedCodec in FramedRead has a length_adjustment of 9, the encoder doesn't seem to make adjustment to the encoded length. For a length adjustment of 9 and a real max_frame_length of N, the LengthDelimitedCodec would expect an encoded length of N-9 (ie. if the encodec length is N-9, the codec would read N-9+9=N bytes). Thus if the encoded length is N, the codec would try to read N+9 which exceeds the max_frame_length and would cause an error.

One of the newly added unit test for the above mentioned PR is attached below as an example

#[test]
fn read_single_frame_positive_length_adjusted_and_max_sized() {
    let mut d: Vec<u8> = vec![];
    d.extend_from_slice(b"\x00\x00\x00\x07Hello world");

    let io = length_delimited::Builder::new()
        .length_field_offset(0)
        .length_field_length(4)
        .max_frame_length(11)
        .length_adjustment(4)
        .new_read(mock! {
            data(&d),
        });
    pin_mut!(io);

    assert_next_eq!(io, b"Hello world");
    assert_done!(io);
}

Workaround (a sketchy one)

This can be circumvented if an additional 9 is added when setting the decoders max_frame_length ie.

    /// Updates the max frame size setting.
    ///
    /// Must be within 16,384 and 16,777,215.
    #[inline]
    pub fn set_max_frame_size(&mut self, val: usize) {
        assert!(DEFAULT_MAX_FRAME_SIZE as usize <= val && val <= MAX_MAX_FRAME_SIZE as usize);
        self.inner.decoder_mut().set_max_frame_length(val + 9)
    }

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the h2_connect_large_body test and the decoder set_max_frame_size method shown in the issue. Trace how FramedRead uses LengthDelimitedCodec alongside the custom FramedWrite encoder, then verify behavior against the linked tokio-util overflow fix. Done means the large-body test passes without relying on the described workaround.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
networking
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.