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

PacketFoobar::packet_size(_packet: &Foobar) is broken #519

Open
WGH- opened this issue Aug 12, 2021 · 1 comment
Open

PacketFoobar::packet_size(_packet: &Foobar) is broken #519

WGH- opened this issue Aug 12, 2021 · 1 comment

Comments

@WGH-
Copy link

WGH- commented Aug 12, 2021

packet_size is supposed to return "The size (in bytes) of an instance when converted into a byte-array".

However, this doesn't work correctly when complex variable length fields are present.

Consider the following test:

#[test]
fn test_tcp_packet_size() {
    let tcp = Tcp{
        source: 36328,
        destination: 443,
        sequence: 2012456774,
        acknowledgement: 825842053,
        data_offset: 8,
        reserved: 0,
        flags: 24,
        window: 32,
        checksum: 39435,
        urgent_ptr: 0,
        options: vec![
            TcpOption::nop(),
            TcpOption::nop(),
            TcpOption::timestamp(0xdeadbeef, 0xdeadbeef),
        ],
        payload: vec![],
    };

    // fixed fields + nop + nop + timestamp
    let expected_length = 20 + 1 + 1 + 10;

    assert_eq!((tcp.data_offset as usize) * 4 + tcp.payload.len(), expected_length);
    assert_eq!(MutableTcpPacket::packet_size(&tcp), expected_length);
}

The test will fail:

thread 'tcp::test_tcp_packet_size' panicked at 'assertion failed: `(left == right)`
  left: `23`,
 right: `32`', src/tcp.rs:464:5

I took a look at how these macros were expanded:

/// The size (in bytes) of a Tcp instance when converted into
/// a byte-array
#[inline]
pub fn packet_size(_packet: &Tcp) -> usize {
    20 + _packet.options.len() + _packet.payload.len()
}

Instead of calculating actual size in bytes, it just returns a number of TCP options, which is wrong.

@WGH-
Copy link
Author

WGH- commented Aug 13, 2021

I suppose this is the part that needs to be fixed:

Type::Vector(_) => {
struct_length = Some(format!("_packet.{}.len()", field_name).to_owned());
if !is_payload && packet_length.is_none() {
return Err(Error::new(
field.ty.span(),
"variable length field must have #[length = \"\"] or \
#[length_fn = \"\"] attribute",
));
}
}

Something roughly like _packet.{}.iter().map(|x| x.packet_size()).sum(), except you can't actually write it this way... I somehow think this macro system shows its inherent limitations. It would be simpler if trait like PacketSize was be defined for primitive types, and it was also defined for Vec<T> where T: PacketSize. This way packet size calculation would be also achieved without ugly macro code generation as well.

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