diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index d58b978..fcc93f4 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -576,6 +576,18 @@ one TCP connection at a time; entity status and vehicle identification requests over TCP are silently dropped; the handler passed to `Server::new` is not validated. +`Message::decode` still accepts a header whose `payload_length` disagrees with +what the payload actually occupies -- it hands the payload exactly that many +bytes and does not require them all to be consumed. `Message::encode` no longer +propagates such a length (it derives the field), so the frame it emits is always +self-consistent, but the lenient decode remains. ISO 13400-2 has an entity +answer an invalid payload length with NACK `0x04`, and +`MessageError::PayloadLengthTooShort` exists, unused, for exactly this. Making +the decode strict is a redesign rather than a fix: `Payload::decode` would have +to report unconsumed bytes, and the identification requests deliberately +discard their EID/VIN body, so a `0x0002` request carrying its 6 EID bytes +would start being rejected outright rather than declined. + `RoutingActivationRequest::encode` omits the optional vehicle-manufacturer field when it is `None`, writing 7 bytes instead of 11. That is what the optionality means, and every golden vector agrees — but no vector exercises a diff --git a/CHANGELOG.md b/CHANGELOG.md index 494a9bb..138d6cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,10 +11,13 @@ This file was reconstructed from the commit and pull-request history when the crate was prepared for publication, so entries before that point describe what changed rather than what was announced at the time. -## [Unreleased] +## [0.6.0] — 2026-09-10 ### Added +- `MessageError::PayloadTooLarge`, for a payload that cannot be described by + the `u32` `payload_length` field. `MessageError` is `#[non_exhaustive]`, so + this is not a breaking change. - `LICENSE-MIT` and `LICENSE-APACHE`. The manifest had declared `MIT OR Apache-2.0` without carrying either text. - `CHANGELOG.md`, `CONTRIBUTING.md`, `SECURITY.md`. @@ -25,6 +28,19 @@ changed rather than what was announced at the time. ### Fixed +- **Breaking:** `Message::encode` derives the header's `payload_length` from + the payload instead of writing the field verbatim. A decoded message keeps + whatever length the wire declared, and `Payload::decode` need not consume + all of it, so re-encoding a frame that arrived with a mismatched length + emitted a frame no decoder would accept. Anything that decodes and re-emits + -- a proxy, a replay tool, a logging fake -- was turning a + malformed-but-accepted frame into a corrupt one. Nothing stops compiling; + what changes is the bytes emitted, which is why this is a minor bump and + not a patch. For such a frame, + `decode(encode(m)).header.payload_length` is now the payload's real size + rather than the length it arrived with. +- `MessageError::PayloadLengthTooShort`'s message said "does match" where it + meant "does not match". - `ClientConnectionInfo::logical_address` carries the address the tester activated routing with, instead of always being `0x0000`. A handler can now tell which tester is asking, and the default `alive_check` answers with the @@ -117,9 +133,10 @@ changed rather than what was announced at the time. Initial implementation: DoIP message types, framing, and an async client and server over tokio. - + -[Unreleased]: https://github.com/luminartech/simple_doip/compare/v0.5.1...HEAD +[0.6.0]: https://github.com/luminartech/simple_doip/compare/v0.5.2...v0.6.0 +[0.5.2]: https://github.com/luminartech/simple_doip/compare/v0.5.1...v0.5.2 [0.5.1]: https://github.com/luminartech/simple_doip/compare/v0.1.0...v0.5.1 [0.1.0]: https://github.com/luminartech/simple_doip/releases/tag/v0.1.0 diff --git a/Cargo.lock b/Cargo.lock index e997eca..9301575 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -427,7 +427,7 @@ dependencies = [ [[package]] name = "simple_doip" -version = "0.5.2" +version = "0.6.0" dependencies = [ "anyhow", "async-trait", diff --git a/Cargo.toml b/Cargo.toml index c9d91da..49e63ec 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "simple_doip" -version = "0.5.2" +version = "0.6.0" edition = "2024" rust-version = "1.88" description = "An ISO 13400-2 (DoIP) implementation with a no_std, zero-copy protocol core and optional async client and server" diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock index 7f27926..9e40197 100644 --- a/fuzz/Cargo.lock +++ b/fuzz/Cargo.lock @@ -256,7 +256,7 @@ checksum = "f8fadd59c855ef2080decdef8ff161eb6661b86933c9d82e5ba29dc602a55aba" [[package]] name = "simple_doip" -version = "0.5.2" +version = "0.6.0" dependencies = [ "async-trait", "automotive-wire-codec", diff --git a/fuzz/fuzz_targets/fuzz_roundtrip.rs b/fuzz/fuzz_targets/fuzz_roundtrip.rs index b1d9db1..37f75e5 100644 --- a/fuzz/fuzz_targets/fuzz_roundtrip.rs +++ b/fuzz/fuzz_targets/fuzz_roundtrip.rs @@ -1,7 +1,7 @@ #![no_main] use libfuzzer_sys::fuzz_target; -use simple_doip::messages::{Decode, Encode, Header, Message}; +use simple_doip::messages::{Decode, Encode, Message}; // The asymmetry hunt: if a frame decodes, re-encoding it and decoding that // again must yield the same message. @@ -19,16 +19,6 @@ fuzz_target!(|data: &[u8]| { let Ok(size) = message.encoded_size() else { return; }; - - // Skip frames whose header declares a length the payload does not actually - // occupy. `decode` accepts those, keeps the declared length verbatim, and - // `encode` then writes it beside a payload of the real size -- emitting a - // frame no decoder will accept. That is luminartech/simple_doip#15, not an - // asymmetry in the field order this target is hunting for, and the check - // comes out when #15 is fixed. - if message.header.payload_length as usize != size - Header::SIZE { - return; - } let mut encoded = vec![0u8; size]; { let mut writer: &mut [u8] = &mut encoded; @@ -40,5 +30,30 @@ fuzz_target!(|data: &[u8]| { let (reparsed, rest) = Message::decode(&encoded).expect("re-decoding an encoded message must not fail"); assert!(rest.is_empty(), "re-encoding left trailing bytes"); - assert_eq!(message, reparsed, "round trip changed the message"); + + // The payload, not the whole message: `encode` derives the header's + // declared length from the payload, so a frame that arrived claiming a + // length its payload did not occupy comes back with the real one. That + // normalization is the fix for #15 -- asserting full `Message` equality + // here would assert the bug back into existence. + assert_eq!( + message.payload, reparsed.payload, + "round trip changed the payload" + ); + assert_eq!( + message.header.payload_type, reparsed.header.payload_type, + "round trip changed the payload type" + ); + + // Encoding is idempotent: having normalized once, a second pass must + // produce the very same bytes. This is the property that would catch a + // field order asymmetry, now that the length no longer masks it. + let mut again = vec![0u8; reparsed.encoded_size().expect("size of a decoded message")]; + { + let mut writer: &mut [u8] = &mut again; + reparsed + .encode(&mut writer) + .expect("re-encode must not fail"); + } + assert_eq!(encoded, again, "encoding is not idempotent"); }); diff --git a/src/messages/message_error.rs b/src/messages/message_error.rs index a3406ae..38ae858 100644 --- a/src/messages/message_error.rs +++ b/src/messages/message_error.rs @@ -43,7 +43,7 @@ pub enum MessageError { /// /// Currently never produced by this crate. #[error( - "Payload length in header does match expected payload type length: {value:?}, expected: {expected:?}" + "Payload length in header does not match expected payload type length: {value:?}, expected: {expected:?}" )] PayloadLengthTooShort { /// The payload length actually declared in the header. @@ -51,6 +51,18 @@ pub enum MessageError { /// The minimum payload length required for the header's `payload_type`. expected: u32, }, + /// The payload encodes to more bytes than the header's `payload_length` + /// field can describe. `DoIP` declares that length as a `u32`, so such a + /// message has no valid wire form. Recoverable: nothing was written. + /// + /// Unreachable for a frame that came off the wire, whose length was itself + /// a `u32`; it takes a hand-built [`Payload`](crate::messages::Payload) + /// borrowing more than `u32::MAX` bytes. + #[error("Payload of {size} bytes exceeds the u32 payload_length field")] + PayloadTooLarge { + /// The payload's encoded size. + size: usize, + }, /// A structurally valid, supported [`PayloadType`] was received in a context /// where it is not a legal response (e.g. a request-only type arriving as a /// response). Recoverable: the frame itself decoded fine. diff --git a/src/messages/mod.rs b/src/messages/mod.rs index d8994ba..c44215b 100644 --- a/src/messages/mod.rs +++ b/src/messages/mod.rs @@ -300,12 +300,34 @@ impl Encode for Message<'_> { Ok(Header::SIZE + self.payload.encoded_size()?) } - /// Serialize this message (header + payload) into `writer` + /// Serialize this message (header + payload) into `writer`. + /// + /// The header's `payload_length` is **derived from the payload** rather + /// than taken from `self.header`. A decoded message keeps whatever length + /// the wire declared, and [`Payload::decode`] is not required to consume + /// all of it -- so writing that field verbatim can emit a frame whose + /// declared length no decoder, including this one, will accept. Deriving + /// it means an encoded frame is always self-consistent. + /// + /// The visible consequence: for a frame that arrived with a declared + /// length the payload did not occupy, `decode(encode(m)).header + /// .payload_length` is the payload's real size, not the length `m` + /// arrived with. /// /// # Errors - /// Returns a [`MessageError`] if the header or payload cannot be serialized + /// Returns a [`MessageError`] if the header or payload cannot be + /// serialized, or [`MessageError::PayloadTooLarge`] if the payload does + /// not fit the `u32` length field. fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { - let written = self.header.encode(writer)?; + let payload_length = self.payload.encoded_size()?; + let header = Header::new( + self.header.protocol_version, + self.header.payload_type, + u32::try_from(payload_length).map_err(|_| MessageError::PayloadTooLarge { + size: payload_length, + })?, + ); + let written = header.encode(writer)?; Ok(written + self.payload.encode(writer)?) } } diff --git a/tests/encode_consistency.rs b/tests/encode_consistency.rs new file mode 100644 index 0000000..7771b79 --- /dev/null +++ b/tests/encode_consistency.rs @@ -0,0 +1,77 @@ +//! `Message::encode` must never emit a frame `Message::decode` rejects. +//! +//! Regression tests for the asymmetry the `fuzz_roundtrip` target found: +//! `decode` takes exactly `header.payload_length` bytes and lets +//! `Payload::decode` consume fewer without complaint, so a decoded `Message` +//! could carry a declared length its payload did not occupy. `encode` wrote +//! that stale field verbatim, producing a frame that failed to decode with +//! `Incomplete`. +//! +//! Both inputs below are frames a peer can actually send. Neither is +//! well-formed, but both are accepted, and being accepted is what put them on +//! the re-encode path. + +use simple_doip::messages::{Decode, Encode, Message, Payload, PayloadType}; + +/// Encode `message` into a fresh buffer sized by `encoded_size`. +fn encode(message: &Message<'_>) -> Vec { + let mut buf = vec![0u8; message.encoded_size().expect("encoded_size failed")]; + { + let mut writer: &mut [u8] = &mut buf; + message.encode(&mut writer).expect("encode failed"); + } + buf +} + +/// A NACK body is one byte. This frame's header claims five. +#[test] +fn a_fixed_size_payload_with_an_overlong_declared_length_still_round_trips() { + let framed: [u8; 13] = [ + 0x02, 0xFD, 0x00, 0x00, 0x00, 0x00, 0x00, 0x05, 0x03, 0x00, 0x00, 0x00, 0x00, + ]; + let (message, _rest) = Message::decode(&framed).expect("the frame is accepted as-is"); + assert_eq!( + message.header.payload_length, 5, + "decode still reports what the wire declared" + ); + + let encoded = encode(&message); + let (reparsed, rest) = Message::decode(&encoded).expect("re-decode must succeed"); + assert!(rest.is_empty()); + assert_eq!( + reparsed.header.payload_length, 1, + "the encoded frame declares the length its payload actually occupies" + ); + assert_eq!(message.payload, reparsed.payload); +} + +/// `VehicleIdentificationRequest` is a unit variant -- an empty body. This +/// frame's header claims one byte, which `Payload::decode` discards. +#[test] +fn a_unit_payload_with_a_nonzero_declared_length_still_round_trips() { + let framed: [u8; 9] = [0x00, 0xFF, 0x00, 0x01, 0x00, 0x00, 0x00, 0x01, 0x00]; + let (message, _rest) = Message::decode(&framed).expect("the frame is accepted as-is"); + assert_eq!(message.payload, Payload::VehicleIdentificationRequest); + assert_eq!(message.header.payload_length, 1); + + let encoded = encode(&message); + assert_eq!(encoded.len(), 8, "a unit payload encodes to a bare header"); + let (reparsed, rest) = Message::decode(&encoded).expect("re-decode must succeed"); + assert!(rest.is_empty()); + assert_eq!(reparsed.header.payload_length, 0); + assert_eq!( + reparsed.header.payload_type, + PayloadType::VehicleIdentificationRequest + ); +} + +/// A well-formed frame is untouched: the derived length equals the declared +/// one, so the bytes are identical. This is what keeps the golden vectors +/// valid. +#[test] +fn a_well_formed_frame_encodes_to_the_bytes_it_came_from() { + let framed: [u8; 9] = [0x02, 0xFD, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x03]; + let (message, rest) = Message::decode(&framed).expect("decode"); + assert!(rest.is_empty()); + assert_eq!(encode(&message), framed); +}