Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
minghuaw opened this issue Jul 9, 2022 · 0 comments

Comments

@minghuaw
Copy link

minghuaw commented Jul 9, 2022

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)
    }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant