From e832216cbbc0f811a53892dcc8d7abbfb2f6b7bc Mon Sep 17 00:00:00 2001 From: Justin Kovacich Date: Wed, 9 Sep 2026 16:06:30 -0400 Subject: [PATCH 1/5] fix(messages): derive the declared payload length when encoding a message `Message::encode` could emit a frame that `Message::decode` rejects. `decode` takes exactly `header.payload_length` bytes and hands them to `Payload::decode`, which is not required to consume all of them. The decoded `Message` keeps the header verbatim, declared length included, and `encode` then wrote that stale field beside a payload of its real size. `encoded_size()` already disagreed with the header being written -- it returns `Header::SIZE + payload.encoded_size()`, not the declared length. A NACK frame whose header claims five body bytes and carries one decodes cleanly, re-encodes to nine bytes with the header still claiming five, and fails to re-decode with `Incomplete { needed: 5, available: 1 }`. Anything that decodes a frame and re-emits it -- a proxy, a replay tool, a logging fake -- was turning a malformed-but-accepted frame into a corrupt one on the wire, and this crate's own `MessageCodec` encoder is on that path. `encode` now builds its header from `payload.encoded_size()`, so an encoded frame is always self-consistent. The visible consequence, documented on the method: for a frame that arrived with a mismatched length, `decode(encode(m)).header.payload_length` is the payload's real size rather than the length it arrived with. A well-formed frame is byte-identical, which is why every golden vector still passes. `MessageError::PayloadTooLarge` covers the one fallible step, a payload too big for the `u32` length field -- unreachable for a frame off the wire, whose length was itself a `u32`. The enum is `#[non_exhaustive]`, so adding it breaks nothing. Also corrects `PayloadLengthTooShort`'s message, which said "does match" where it meant "does not match". That variant is still never produced; it is the natural home for the decode half of this, which stays deferred -- see ARCHITECTURE.md. Found by the `fuzz_roundtrip` target in under a second. Closes #15. Co-Authored-By: Claude Opus 5 (1M context) --- src/messages/message_error.rs | 14 +++++++++++++- src/messages/mod.rs | 28 +++++++++++++++++++++++++--- 2 files changed, 38 insertions(+), 4 deletions(-) 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)?) } } From b6e5c90ea3050c2182593759441cc740520652d2 Mon Sep 17 00:00:00 2001 From: Justin Kovacich Date: Wed, 9 Sep 2026 16:06:50 -0400 Subject: [PATCH 2/5] test: pin the encode/decode consistency the fix restores Three cases, all frames a peer can actually send: - A NACK whose header declares five body bytes and carries one. Decode still reports the declared 5; the re-encoded frame declares 1 and decodes. - A `VehicleIdentificationRequest`, a unit variant, whose header declares one byte. `Payload::decode` discards it by design, so the payload is empty and the re-encoded frame is a bare 8-byte header. - A well-formed frame, which must encode back to the exact bytes it came from. That third case is what keeps the golden vectors honest -- the fix must not touch a frame whose declared length was already right. Co-Authored-By: Claude Opus 5 (1M context) --- tests/encode_consistency.rs | 77 +++++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 tests/encode_consistency.rs 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); +} From 100af5963b7249749640395d98c4d831ce753db3 Mon Sep 17 00:00:00 2001 From: Justin Kovacich Date: Wed, 9 Sep 2026 16:06:50 -0400 Subject: [PATCH 3/5] test(fuzz): drop the #15 skip and assert encoding is idempotent The target skipped frames whose declared length disagreed with the payload, because asserting the round trip on those asserted the bug. With #15 fixed the skip comes out. The assertion is payload and payload-type equality rather than whole-`Message` equality: `encode` now derives the declared length, so a frame that arrived with a bogus one legitimately comes back with the real one. Asserting full equality would assert the bug back into existence. Idempotence replaces what that equality was buying -- encode, decode, encode again, and the bytes must be identical. Having normalized once, a second pass cannot differ, and that is the property that catches a field written in one order and read in another now that the length no longer masks it. 21 million executions clean, plus 6.4M, 8.1M and 1.7M on the other three. Co-Authored-By: Claude Opus 5 (1M context) --- fuzz/fuzz_targets/fuzz_roundtrip.rs | 39 ++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 12 deletions(-) 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"); }); From 87a27ec1d702d7c69b5bf520835e47db83a728de Mon Sep 17 00:00:00 2001 From: Justin Kovacich Date: Wed, 9 Sep 2026 16:06:50 -0400 Subject: [PATCH 4/5] docs: record the fix and the decode-side question it leaves open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The changelog gets the behavior change and the new error variant, because a consumer deciding whether to upgrade needs the consequence spelled out: for a frame that arrived with a mismatched declared length, the length after a round trip is the payload's real size, not the one it arrived with. ARCHITECTURE.md §7.6 records what the fix deliberately does not address. `Message::decode` still accepts a header whose `payload_length` disagrees with what the payload occupies. ISO 13400-2 has an entity answer that with NACK `0x04`, and `MessageError::PayloadLengthTooShort` sits unused for exactly it -- but making the decode strict is a redesign, not 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 six EID bytes would start being rejected outright rather than declined. Co-Authored-By: Claude Opus 5 (1M context) --- ARCHITECTURE.md | 12 ++++++++++++ CHANGELOG.md | 14 ++++++++++++++ 2 files changed, 26 insertions(+) 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..f4fb06c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,9 @@ changed rather than what was announced at the time. ### 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,17 @@ changed rather than what was announced at the time. ### Fixed +- `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. Consequence to know about: 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 From 7215582ee97ab165eb5dfcb994438cf2e4519c27 Mon Sep 17 00:00:00 2001 From: Justin Kovacich Date: Thu, 10 Sep 2026 09:38:39 -0400 Subject: [PATCH 5/5] chore(release): v0.6.0 The stack from #11 breaks no signature -- `MessageError` is `#[non_exhaustive]`, so the added `PayloadTooLarge` variant is additive, and `ClientConnectionInfo::logical_address` keeps its type and only starts carrying a real value. What earns the minor bump is `Message::encode` deriving the header's declared length: a caller that set a mismatched `payload_length` on purpose stops being able to emit that frame, and nothing in the type system says so. A silent change in emitted bytes is the case the 0.x minor bump exists for, so the CHANGELOG entry is marked breaking to match the version. v0.5.2 is now tagged at the #10 merge on main, so the section that had no comparison range gets one, and the release links run 0.5.2...0.6.0. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 25 ++++++++++++++----------- Cargo.lock | 2 +- Cargo.toml | 2 +- fuzz/Cargo.lock | 2 +- 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4fb06c..138d6cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ 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 @@ -28,13 +28,15 @@ changed rather than what was announced at the time. ### Fixed -- `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. Consequence to know about: for such a frame, +- **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 @@ -131,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",