Skip to content

fix(messages): derive the declared payload length when encoding a message - #17

Merged
JustinKovacich merged 5 commits into
test/port-property-and-fuzz-suitesfrom
fix/encode-derives-declared-payload-length
Sep 10, 2026
Merged

fix(messages): derive the declared payload length when encoding a message#17
JustinKovacich merged 5 commits into
test/port-property-and-fuzz-suitesfrom
fix/encode-derives-declared-payload-length

Conversation

@JustinKovacich

@JustinKovacich JustinKovacich commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Stacked on #16 — merge order: #11#12#13#16 → this.

Issue URL

Closes #15.

What

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
wrote that stale field beside a payload of its real size. Note that
encoded_size() already disagreed with the header being written — it returns
Header::SIZE + payload.encoded_size(), not the declared length.

// A NACK body is one byte. This header claims five.
let framed = [0x02, 0xFD, 0, 0, 0, 0, 0, 0x05, 0x03, 0, 0, 0, 0];
let (msg, _) = Message::decode(&framed).unwrap();   // accepted
assert_eq!(msg.header.payload_length, 5);           // preserved
// re-encode -> 9 bytes, header still claims 5
Message::decode(&encode(&msg));  // Err(Incomplete { needed: 5, available: 1 })

Anything that decodes a frame and re-emits it — a proxy, a replay tool, a
logging fake, a test harness echoing what it received — was turning a
malformed-but-accepted frame into a corrupt one on the wire. This crate's own
MessageCodec encoder is on that path.

The fix

encode builds its header from payload.encoded_size() rather than trusting
self.header.payload_length, so an encoded frame is always self-consistent
regardless of how lenient decode is. No input that is accepted today starts
being rejected.

The consequence, documented on the method: for a frame that arrived with a
mismatched declared length, decode(encode(m)).header.payload_length is the
payload's real size rather than the length it arrived with. That is the point —
but it is a visible behavior change, so it's in the changelog.

A well-formed frame is byte-identical, which is why all 11 golden vectors
still pass unchanged. That's also pinned as a test.

MessageError::PayloadTooLarge covers the one fallible step — a payload too
large for the u32 length field, unreachable for a frame off the wire whose
length was itself a u32. MessageError is #[non_exhaustive], so adding it
breaks nothing.

Why not make decode strict instead

That is the standards-correct complement — ISO 13400-2 has an entity answer an
invalid payload length with NACK 0x04, and MessageError::PayloadLengthTooShort
sits unused for exactly this. But it's a redesign, not a fix: Payload::decode
would have to report unconsumed bytes, and the identification requests
deliberately discard their EID/VIN body (ARCHITECTURE.md §7.6), so a
0x0002 request carrying its six EID bytes would start being rejected outright
rather than declined — breaking the UDP responder path.

Recorded in ARCHITECTURE.md §7.6 as deferred rather than dropped.

Testing

tests/encode_consistency.rs 3 regression cases: overlong length on a fixed payload, nonzero length on a unit payload, and a well-formed frame encoding to the bytes it came from
golden vectors 11/11 pass unchanged — the fix cannot touch a frame whose declared length was already right
fuzz_roundtrip, skip removed, idempotence asserted 21,033,263 executions clean
other three fuzz targets 6.4M / 8.1M / 1.7M clean
full test suite, all features and none pass
clippy --all-targets --all-features -Dclippy::pedantic, and --no-default-features clean
cargo doc with -D warnings, cargo fmt, pre-commit, cargo publish --dry-run, MSRV 1.88 pass

The fuzz target now asserts payload and payload-type equality plus
idempotence (encode, decode, encode again → identical bytes) rather than
whole-Message equality. Asserting full equality would assert the bug back
into existence, since the declared length legitimately normalizes; idempotence
buys back the field-order asymmetry detection that equality was providing.

Also

Corrects PayloadLengthTooShort's message, which read "does match" where it
meant "does not match" — a user-visible error string.

Release bump: 0.6.0

This PR sits at the top of the stack, so it also carries
chore(release): v0.6.0 — the version the whole stack (#11#17) publishes
as. It follows the same pattern as 0.5.2, whose bump was made inside #10's
branch rather than by cargo release on main.

Nothing in the stack breaks a signature: MessageError is
#[non_exhaustive], so PayloadTooLarge is additive, and
ClientConnectionInfo::logical_address keeps its type and only starts
carrying a real value. The bump is for the encode change in this PR — a
caller that set a mismatched payload_length deliberately (a negative-test
fake, a corpus generator, a proxy replaying what it saw) stops being able to
emit that frame, with no compiler diagnostic anywhere. The CHANGELOG entry is
marked Breaking: so the version and the section header tell the same
story.

v0.5.2 is now tagged at the #10 merge on main (304d014), so that
section has a comparison range and the release links run
v0.5.2...v0.6.0.

Review status

Not reviewed by anyone yet. Draft.

JustinKovacich and others added 5 commits September 10, 2026 09:54
…sage

`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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
@JustinKovacich
JustinKovacich force-pushed the fix/encode-derives-declared-payload-length branch from 5e1c081 to 7215582 Compare September 10, 2026 13:55
@JustinKovacich
JustinKovacich marked this pull request as ready for review September 10, 2026 16:41
@JustinKovacich
JustinKovacich merged commit 30ba912 into main Sep 10, 2026
18 checks passed
@JustinKovacich
JustinKovacich deleted the fix/encode-derives-declared-payload-length branch September 10, 2026 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Message::encode can emit a frame Message::decode rejects, when the header's declared length disagrees with the payload

2 participants