hyperium / h2

HTTP 2.0 client & server implementation for Rust.
MIT License
1.34k stars 269 forks source link

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

Open minghuaw opened 2 years ago

minghuaw commented 2 years ago

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)
    }