tokio-rs / tokio

A runtime for writing reliable asynchronous applications with Rust. Provides I/O, networking, scheduling, timers, ...
https://tokio.rs
MIT License
25.73k stars 2.35k forks source link

LengthDelimitedCodec misses that last N bytes of the frame with num_skip(0) #6604

Closed erenon closed 3 weeks ago

erenon commented 1 month ago

Version

tokio master branch (14c17fc09656a30230177b600bacceb9db33e942, tokio 1.38.0), also tokio-util 0.7.11.

Platform

Linux

Description

The documentation gives this example:

LengthDelimitedCodec::builder()
    .length_field_offset(0) // default value
    .length_field_type::<u16>()
    .length_adjustment(0)   // default value
    .num_skip(0) // Do not strip frame header
    .new_read(io);

The following frame will be decoded as such:

         INPUT                           DECODED
+-- len ---+--- Payload ---+     +-- len ---+--- Payload ---+
| \x00\x0B |  Hello world  | --> | \x00\x0B |  Hello world  |
+----------+---------------+     +----------+---------------+

However, the following (new) test fails in tokio-util/tests/length_delimited.rs:

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

    let io = length_delimited::Builder::new()
        .length_field_offset(0)
        .length_field_type::<u16>()
        .length_adjustment(0)
        .num_skip(0)
        .new_read(mock! {
            data(&d),
        });
    pin_mut!(io);

    assert_next_eq!(io, b"\x00\x0BHello world");
    assert_done!(io);
}

Error message:

thread 'read_single_frame_skip_none' panicked at tokio-util/tests/length_delimited.rs:389:5:
assertion `left == right` failed
  left: b"\0\x0bHello wor"
 right: [0, 11, 72, 101, 108, 108, 111, 32, 119, 111, 114, 108, 100]

The last two bytes of "world" is missing. With a longer size prefix (u32), the number of missing byte increases (4). It appears to me that the logic that reads the size advances the pointer, but this is incorrectly adjusted later, when num_skip(0) is taken into account.

A possible workaround:

        .length_adjustment(2) # 2 must match the size prefix size.
name1e5s commented 1 month ago

According to this test, it seems that use length_adjustment to adjust result length is an expected behavior when .num_skip(0) is set.

https://github.com/tokio-rs/tokio/blob/16fccafb416eb7dd0e324114886cf94021e208ff/tokio-util/tests/length_delimited.rs#L351-L372

We may need to update the example in documentation.