From 50061be0aff49470607533f68a5d058224427e97 Mon Sep 17 00:00:00 2001 From: Matt <47545907+SoundMatt@users.noreply.github.com> Date: Wed, 29 Jul 2026 08:27:37 -0700 Subject: [PATCH] fix: reject oversized payloads at LIN publish paths; correct E2E manual MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SAFETY_MANUAL.md documented the canonical ASIL-B E2E-protected publish pattern as safety::Protector::protect() output fed directly into Bus::do_publish()/slave::Node::set_response(). protect() always prepends a fixed kHeaderSize (10) byte header, so its output is always >= 10 bytes -- but kLINMaxDataLen (8) already caps a LIN frame's data field below that, for any payload including a zero-byte one. None of the actual publish paths validated data.size() at all, so the documented "safe" pattern silently produced out-of-spec frames instead of failing loudly. Fixes: - Bus::do_publish() now rejects any non-empty payload longer than kLINMaxDataLen with ErrInvalidFrame (empty payload keeps its existing "unregister this ID" meaning and is unaffected). slave::Node:: set_response() delegates to do_publish() and is covered transitively. - The RELAY adapter's LinAdapter::send() translates that same condition to ErrPayloadTooLarge before it would otherwise leak lin::Errc::invalid_frame through relay::INode::send(), whose documented contract (spec §10.1) only allows ErrClosed, ErrNotConnected, ErrTimeout, or ErrPayloadTooLarge. - SAFETY_MANUAL.md §4.2 no longer presents the broken single-frame pattern as usable. It now states the real numeric constraint, explains why no payload size makes it fit, and documents that Protector/Receiver are for use over a transport capable of carrying more than 8 bytes per logical message (e.g. a future multi-frame LIN diagnostic transport -- not yet implemented, see ROADMAP.md) rather than direct single-frame publish -- with a corrected example that never calls Bus::publish()/set_response() on protected output. Closes #17 Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com> --- SAFETY_MANUAL.md | 48 ++++++++++++++++++++++++++++-------- src/lin.cpp | 9 +++++++ src/virtual/bus.cpp | 12 +++++++++ tests/test_relay_adapter.cpp | 16 ++++++++++++ tests/test_slave.cpp | 12 +++++++++ tests/test_virtual.cpp | 18 ++++++++++++++ 6 files changed, 105 insertions(+), 10 deletions(-) diff --git a/SAFETY_MANUAL.md b/SAFETY_MANUAL.md index b1159fb..1376512 100644 --- a/SAFETY_MANUAL.md +++ b/SAFETY_MANUAL.md @@ -101,21 +101,49 @@ process_frame(hw_frame); ### 4.2 E2E Protection for Safety-Critical Payloads (SG-05) -Use `lin::safety::Protector` and `Receiver` for all ASIL-B data paths: +**`lin::safety::Protector`/`Receiver` cannot protect a payload published as a +single raw LIN frame.** `Protector::protect()` always prepends a fixed +`kHeaderSize` (10-byte) header to the payload (`include/lin/safety/e2e.hpp`), +so its output is always at least 10 bytes — but a LIN frame's data field is +capped at `kLINMaxDataLen` (8 bytes). This is true for *any* input payload, +including a zero-byte one: there is no payload size for which +`protect()`'s output fits in a single LIN frame. `Bus::do_publish()` and +`slave::Node::set_response()` both reject any non-empty payload longer than +`kLINMaxDataLen` with `ErrInvalidFrame` (and the RELAY adapter's `send()` +surfaces the equivalent `ErrPayloadTooLarge`) specifically so this mistake +fails loudly instead of silently truncating or corrupting the frame on the +wire — **do not** work around that rejection (e.g. by truncating +`protect()`'s output yourself); a truncated E2E-protected payload is not a +valid protected payload. + +As of this release, `lin::safety::Protector`/`Receiver` are **not usable +for direct single-frame LIN publish** and must not be wired into +`Bus::do_publish()` / `slave::Node::set_response()` this way. The module is +retained for callers who protect a payload that travels over a transport +capable of carrying more than 8 bytes per logical message — e.g. a future +multi-frame LIN diagnostic transport (see `ROADMAP.md`'s planned +`UDS (ISO 14229) over LIN TP adapter`, not yet implemented) — where the +protected payload is fragmented across several physical LIN frames by that +transport layer, not published directly. Round-trip `protect()`/`unwrap()` +usage that never goes through `IBus::publish()` (e.g. protecting a payload +before handing it to your own transport) remains safe and thread-safe: ```cpp -// Sender side (e.g., sensor ECU) lin::safety::Config cfg{.data_id = 0x0042, .source_id = 0x0001}; lin::safety::Protector protector{cfg}; +lin::safety::Receiver receiver{cfg}; -auto raw_payload = read_sensor_value(); -auto safe_payload = protector.protect(raw_payload); -bus->publish(FRAME_ID_SENSOR, safe_payload); +auto raw_payload = read_sensor_value(); +auto safe_payload = protector.protect(raw_payload); // >= 10 bytes — do NOT + // pass this to + // Bus::publish()/ + // set_response() + // directly + +// ... safe_payload travels over your own multi-frame-capable transport ... -// Receiver side (e.g., actuator ECU) -lin::safety::Receiver receiver{cfg}; try { - auto verified = receiver.unwrap(frame.data); + auto verified = receiver.unwrap(safe_payload); actuate(verified); } catch (const lin::safety::E2EError& e) { // mandatory safe state on E2E failure @@ -245,7 +273,7 @@ must ensure: 4. The `id` field in `relay::Message` carries the decimal string representation of the LIN frame ID (e.g., `"16"` for frame 0x10). -See `RELAY Spec v1.11 §8.3` for the complete LIN-over-RELAY envelope specification. +See `RELAY Spec v1.14 §8.3` for the complete LIN-over-RELAY envelope specification. --- @@ -290,4 +318,4 @@ Reproducing from `SEOOC.md` for convenience: - `sas.md` — Software Architecture Specification - ISO 26262:2018 Part 6 §7 — Software integration and verification - ISO 26262:2018 Part 10 §9 — Safety element out of context -- RELAY Specification v1.11 §8.3 — LIN bus binding +- RELAY Specification v1.14 §8.3 — LIN bus binding diff --git a/src/lin.cpp b/src/lin.cpp index 2bece40..d0659c3 100644 --- a/src/lin.cpp +++ b/src/lin.cpp @@ -160,6 +160,15 @@ class LinAdapter : public relay::INode { } catch (const ErrInvalidFrame&) { return relay::make_error_code(relay::Errc::payload_too_large); } + // Same reasoning as the ID-validation comment above: bus_->publish() + // (IBus::publish() -> Bus::do_publish()) correctly rejects an + // over-length payload with lin::Errc::invalid_frame, but that isn't + // one of relay.Node::send()'s four documented sentinels, so an + // oversized payload is translated to ErrPayloadTooLarge here rather + // than forwarding IBus's raw error code (cpp-LIN#17). + if (f.data.size() > kLINMaxDataLen) { + return relay::make_error_code(relay::Errc::payload_too_large); + } return bus_->publish(f.id, std::move(f.data)); } diff --git a/src/virtual/bus.cpp b/src/virtual/bus.cpp index 2c7626a..daa6035 100644 --- a/src/virtual/bus.cpp +++ b/src/virtual/bus.cpp @@ -36,6 +36,18 @@ std::error_code Bus::do_publish(uint8_t id, std::vector data, ChecksumT return lin::make_error_code(lin::Errc::invalid_frame); } + // An empty payload is a legitimate "unregister this ID's response" + // signal (handled below), not a length violation — only a non-empty, + // over-length payload is rejected. Without this, callers following + // SAFETY_MANUAL.md's E2E-protected publish pattern (whose + // safety::Protector::protect() output is always >= kHeaderSize (10) + // bytes, already exceeding kLINMaxDataLen (8)) would have an oversized + // frame silently accepted here instead of rejected (cpp-LIN#17). + if (!data.empty() && data.size() > kLINMaxDataLen) { + error_count_.fetch_add(1); + return lin::make_error_code(lin::Errc::invalid_frame); + } + std::unique_lock lk(mu_); if (closed_) { error_count_.fetch_add(1); diff --git a/tests/test_relay_adapter.cpp b/tests/test_relay_adapter.cpp index a7be2dd..28c45fe 100644 --- a/tests/test_relay_adapter.cpp +++ b/tests/test_relay_adapter.cpp @@ -63,6 +63,22 @@ TEST_CASE("adapt: send rejects non-numeric ID string", "[adapter][REQ-ADAPT-003] (void)bus->close(); } +TEST_CASE("adapt: send rejects oversized payload with ErrPayloadTooLarge", "[adapter][REQ-ADAPT-002][REQ-ADAPT-003]") { + // relay::INode::send() (§10.1) may only return ErrClosed, ErrNotConnected, + // ErrTimeout, or ErrPayloadTooLarge — an oversized payload must surface + // as ErrPayloadTooLarge specifically, not the lin::Errc::invalid_frame + // that IBus::publish() returns for the same condition (cpp-LIN#17). + auto bus = Bus::create(); + auto node = adapt(bus); + + relay::Message msg; + msg.id = "16"; + msg.payload = std::vector(kLINMaxDataLen + 1, 0xAA); + auto err = node->send(msg); + CHECK(err == relay::ErrPayloadTooLarge()); + (void)bus->close(); +} + TEST_CASE("adapt: subscribe delivers frames as relay::Message", "[adapter][REQ-ADAPT-004]") { auto bus = Bus::create(); auto node = adapt(bus); diff --git a/tests/test_slave.cpp b/tests/test_slave.cpp index c6833d7..d7e5e26 100644 --- a/tests/test_slave.cpp +++ b/tests/test_slave.cpp @@ -66,6 +66,18 @@ TEST_CASE("set_response rejects ID > 0x3F", "[slave][REQ-SLAVE-004]") { bus->close(); } +TEST_CASE("set_response rejects payload longer than kLINMaxDataLen", "[slave][REQ-SLAVE-002][REQ-SLAVE-004]") { + // set_response() delegates to bus_->publish() (cpp-LIN#17): an oversized + // payload must be rejected here too, not just at the virtual::Bus level. + auto bus = Bus::create(); + Node node(bus); + std::vector oversized(kLINMaxDataLen + 1, 0xAA); + auto err = node.set_response(0x10, oversized); + CHECK(err); + CHECK(node.registered_ids().empty()); + bus->close(); +} + TEST_CASE("registered_ids reflects current state", "[slave][REQ-SLAVE-005]") { auto bus = Bus::create(); Node node(bus); diff --git a/tests/test_virtual.cpp b/tests/test_virtual.cpp index 97ec4d8..c998479 100644 --- a/tests/test_virtual.cpp +++ b/tests/test_virtual.cpp @@ -105,6 +105,24 @@ TEST_CASE("publish rejects ID > 0x3F", "[virtual][REQ-VIRT-004]") { bus->close(); } +TEST_CASE("publish rejects payload longer than kLINMaxDataLen", "[virtual][REQ-VIRT-004]") { + auto bus = Bus::create(); + std::vector oversized(kLINMaxDataLen + 1, 0xAA); + auto err = bus->publish(0x10, oversized); + CHECK(err); // must reject, not silently accept + // and must not have registered a (truncated or otherwise) response + auto [f, herr] = bus->send_header(0x10); + CHECK(herr); // ErrNoResponse: nothing was ever published for 0x10 + bus->close(); +} + +TEST_CASE("publish accepts payload exactly kLINMaxDataLen", "[virtual][REQ-VIRT-004]") { + auto bus = Bus::create(); + std::vector maxsize(kLINMaxDataLen, 0xAA); + CHECK_FALSE(bus->publish(0x10, maxsize)); + bus->close(); +} + TEST_CASE("publish(id, {}) removes registration", "[virtual][REQ-VIRT-003]") { auto bus = Bus::create(); REQUIRE_FALSE(bus->publish(0x10, {0xAA}));