From bda4a3cf48b3d6cf8a5d0978c9b84111246039bf Mon Sep 17 00:00:00 2001 From: Matt <47545907+SoundMatt@users.noreply.github.com> Date: Fri, 31 Jul 2026 06:19:00 -0700 Subject: [PATCH] =?UTF-8?q?fix:=20v0.4.2=20=E2=80=94=20RELAY=20ecosystem?= =?UTF-8?q?=20conformance=20fixes,=20pass=203?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - virtual::Bus::publish() (and the default IBus::publish() path) now forces the classic checksum for diagnostic frame IDs 0x3C/0x3D regardless of the caller's requested checksum type (LIN 2.2A §2.3.1.5, RELAY §15.3), matching validate_frame()'s existing rejection of enhanced-checksum diagnostic frames. - RELAY adapter LinAdapter::send() now honours the bridged message's lin.checksum_type (and forces classic for 0x3C/0x3D), routing to publish_classic() instead of unconditionally upgrading every bridged frame to enhanced. - master::Node::run() now returns success (no-op) for an empty schedule table instead of lin::Errc::invalid_frame, matching RELAY §8.3 ("an empty table is valid and disables scheduled transmission"). Updated REQ-MASTER-009/010 requirement text (both req JSON files) to match. - LDF parser clamps Signal::bit_width to [0, 64] at parse time so DB::decode()'s bit-extraction loop can't be driven into a shift-by->=64 (UB) by a malformed/adversarial LDF file with an oversized payload. - CI: SHA-pin ilammy/msvc-dev-cmd and softprops/action-gh-release instead of floating major-version tags; stop masking cpfusa init / hara init failures with `|| true` so a broken init hard-fails instead of letting downstream FuSa/HARA steps run against a stale config. Regression tests added for all four code fixes, including a malformed-LDF test that reproduces the bit-width UB under UBSan pre-fix. Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com> --- .fusa-reqs.json | 12 ++++----- .github/workflows/ci.yml | 8 +++--- .github/workflows/release.yml | 4 +-- CHANGELOG.md | 26 ++++++++++++++++++ CMakeLists.txt | 2 +- ROADMAP.md | 19 +++++++++++++- requirements/requirements.json | 4 +-- src/ldf/parser.cpp | 10 +++++++ src/lin.cpp | 8 ++++++ src/master/node.cpp | 5 +++- src/virtual/bus.cpp | 7 +++++ tests/test_ldf.cpp | 48 ++++++++++++++++++++++++++++++++++ tests/test_master.cpp | 9 ++++--- tests/test_relay_adapter.cpp | 48 ++++++++++++++++++++++++++++++++++ tests/test_virtual.cpp | 23 ++++++++++++++++ 15 files changed, 213 insertions(+), 20 deletions(-) diff --git a/.fusa-reqs.json b/.fusa-reqs.json index 335b6fb..caa85a0 100644 --- a/.fusa-reqs.json +++ b/.fusa-reqs.json @@ -507,18 +507,18 @@ }, { "id": "REQ-MASTER-009", - "title": "Run returns error for empty schedule", - "description": "Run shall return an error immediately when the schedule table is empty.", + "title": "Run treats an empty schedule as a no-op success", + "description": "Run shall return success immediately (without invoking the bus) when the schedule table is empty, rather than returning an error.", "asil": "ASIL-B", - "rationale": "An empty schedule is a programming error; running it would loop forever.", + "rationale": "RELAY §8.3: an empty schedule table is valid and simply disables scheduled transmission; it is not a programming error, so Run must not fail callers that have not yet configured a schedule.", "tags": ["master", "validation"] }, { "id": "REQ-MASTER-010", - "title": "SetSchedule rejects empty schedule", - "description": "SetSchedule shall return an error when called with nil or a zero-length slice.", + "title": "SetSchedule accepts an empty schedule", + "description": "SetSchedule shall accept a nil or zero-length slice and succeed, disabling scheduled transmission (REQ-MASTER-009) until a non-empty schedule is set.", "asil": "ASIL-B", - "rationale": "REQ-MASTER-009 precondition enforced at configuration time.", + "rationale": "RELAY §8.3: an empty schedule table is valid; SetSchedule must not reject it.", "tags": ["master", "validation"] }, { diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e6170d1..40d3909 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -55,7 +55,7 @@ jobs: - name: Set up MSVC environment (Windows) if: runner.os == 'Windows' - uses: ilammy/msvc-dev-cmd@v1 + uses: ilammy/msvc-dev-cmd@0b201ec74fa43914dc39ae48a89fd1d8cb592756 # v1.13.0 - name: Configure shell: bash @@ -249,7 +249,7 @@ jobs: - name: cpfusa init working-directory: cpp-LIN - run: ../cpp-FuSa/build/cpfusa init --name cpp-LIN --standard iso26262 --asil ASIL-B --project-version 0.4.0 --force || true + run: ../cpp-FuSa/build/cpfusa init --name cpp-LIN --standard iso26262 --asil ASIL-B --project-version 0.4.0 --force - name: cpfusa check working-directory: cpp-LIN @@ -273,7 +273,7 @@ jobs: - name: cpfusa hara init working-directory: cpp-LIN - run: ../cpp-FuSa/build/cpfusa hara init --project cpp-LIN --dir . || true + run: ../cpp-FuSa/build/cpfusa hara init --project cpp-LIN --dir . - name: cpfusa iso26262 (ASIL-B) working-directory: cpp-LIN @@ -450,7 +450,7 @@ jobs: - name: cpfusa init working-directory: cpp-LIN - run: ../cpp-FuSa/build/cpfusa init --name cpp-LIN --standard iso26262 --asil ASIL-B --project-version 0.4.0 --force || true + run: ../cpp-FuSa/build/cpfusa init --name cpp-LIN --standard iso26262 --asil ASIL-B --project-version 0.4.0 --force - name: Generate SARIF report working-directory: cpp-LIN diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index da8bb07..b6b70fb 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -31,7 +31,7 @@ jobs: - name: Set up MSVC (Windows) if: runner.os == 'Windows' - uses: ilammy/msvc-dev-cmd@v1 + uses: ilammy/msvc-dev-cmd@0b201ec74fa43914dc39ae48a89fd1d8cb592756 # v1.13.0 - name: Configure shell: bash @@ -88,7 +88,7 @@ jobs: cat SHA256SUMS.txt - name: Create GitHub Release - uses: softprops/action-gh-release@v2 + uses: softprops/action-gh-release@3bb12739c298aeb8a4eeaf626c5b8d85266b0e65 # v2.6.2 with: name: "cpp-LIN ${{ github.ref_name }}" tag_name: ${{ github.ref_name }} diff --git a/CHANGELOG.md b/CHANGELOG.md index ca9f2d9..71f5ad0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,32 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.4.2] +### Fixed +- `virtual::Bus::publish()` (and the default `IBus::publish()` entry point) + now forces the classic checksum for diagnostic frame IDs 0x3C/0x3D + regardless of the caller's requested checksum type, matching LIN 2.2A + §2.3.1.5 and RELAY §15.3, and matching `validate_frame()`'s existing + rejection of enhanced-checksum diagnostic frames. +- RELAY adapter `LinAdapter::send()` now honours the bridged message's + `lin.checksum_type` (and forces classic for 0x3C/0x3D), routing to + `publish_classic()` instead of unconditionally upgrading every bridged + frame to the enhanced checksum via `publish()`. +- `master::Node::run()` now returns success (a no-op) for an empty schedule + table instead of `lin::Errc::invalid_frame`, matching RELAY §8.3 ("an + empty table is valid and disables scheduled transmission"). +- LDF parser now clamps `Signal::bit_width` to `[0, 64]` at parse time, so + `DB::decode()`'s bit-extraction loop can no longer be driven into a + shift-by->=64 (undefined behaviour) by a malformed/adversarial LDF file + combined with an oversized payload. +### Changed +- CI: `ilammy/msvc-dev-cmd` and `softprops/action-gh-release` pinned to + commit SHA instead of a floating major-version tag. +- CI: `cpfusa init` / `cpfusa hara init` steps no longer swallow failures + with `|| true` — a failed init step now hard-fails the job instead of + letting every downstream FuSa/HARA step silently run against a + missing/stale config. + ## [0.4.1] ### Added - `verify_checksum()`: validates a received checksum byte against the diff --git a/CMakeLists.txt b/CMakeLists.txt index ae5aa7f..fd61a27 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,7 +1,7 @@ cmake_minimum_required(VERSION 3.21) project(cpplin - VERSION 0.4.1 + VERSION 0.4.2 DESCRIPTION "C++ LIN bus library — RELAY-conformant, ISO 26262 ASIL-B" HOMEPAGE_URL "https://github.com/SoundMatt/cpp-LIN" LANGUAGES CXX diff --git a/ROADMAP.md b/ROADMAP.md index 210083b..19d08ff 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -50,7 +50,7 @@ convention), matching peers like `go-can` - [x] `lin::mock` canonical module (spec §13.7.1), aliasing `lin::virt::Bus` -## v0.4.1 — RELAY ecosystem audit fixes, pass 2 (current) +## v0.4.1 — RELAY ecosystem audit fixes, pass 2 - [x] Declared RELAY spec version bumped 1.11 → 2.0 - [x] `verify_checksum()` added; HARA SG-03 downgraded from "Implemented" to @@ -70,6 +70,23 @@ fixed, HARA §15→§5 citation fixed - [x] `CHANGELOG.md` added +## v0.4.2 — RELAY ecosystem audit fixes, pass 3 (current) + +- [x] `virtual::Bus::publish()` forces classic checksum for diagnostic + frames 0x3C/0x3D (LIN 2.2A §2.3.1.5 / RELAY §15.3) +- [x] RELAY adapter `LinAdapter::send()` honours `lin.checksum_type` / + diagnostic-ID classic-checksum override instead of always upgrading + bridged frames to enhanced +- [x] `master::Node::run()` treats an empty schedule as a no-op success + (spec §8.3), not `invalid_frame` +- [x] LDF parser clamps `Signal::bit_width` to `[0, 64]` at parse time + (defence-in-depth against a malformed/adversarial LDF driving + `DB::decode()`'s bit-extraction loop into UB) +- [x] CI: third-party actions (`ilammy/msvc-dev-cmd`, + `softprops/action-gh-release`) pinned to commit SHA +- [x] CI: `cpfusa init` / `cpfusa hara init` steps hard-fail instead of + being masked with `|| true` + ## v0.5.0 — Enhanced Bus Features - [ ] Sleep/wake frame support (LIN 2.x go-to-sleep command) diff --git a/requirements/requirements.json b/requirements/requirements.json index c899dd7..4a296ac 100644 --- a/requirements/requirements.json +++ b/requirements/requirements.json @@ -71,8 +71,8 @@ {"id": "REQ-MASTER-006", "title": "run invokes on_frame on success", "asil": "ASIL-B", "tags": ["master", "callback"]}, {"id": "REQ-MASTER-007", "title": "run invokes on_error on failure", "asil": "ASIL-B", "tags": ["master", "callback"]}, {"id": "REQ-MASTER-008", "title": "run returns on stop token", "asil": "ASIL-B", "tags": ["master", "lifecycle"]}, - {"id": "REQ-MASTER-009", "title": "run returns error for empty schedule", "asil": "ASIL-B", "tags": ["master", "validation"]}, - {"id": "REQ-MASTER-010", "title": "set_schedule rejects empty schedule", "asil": "ASIL-B", "tags": ["master", "validation"]}, + {"id": "REQ-MASTER-009", "title": "run is a no-op success for empty schedule", "asil": "ASIL-B", "tags": ["master", "validation"]}, + {"id": "REQ-MASTER-010", "title": "set_schedule accepts empty schedule", "asil": "ASIL-B", "tags": ["master", "validation"]}, {"id": "REQ-MASTER-011", "title": "set_schedule rejects invalid frame ID", "asil": "ASIL-B", "tags": ["master", "validation"]}, {"id": "REQ-MASTER-012", "title": "set_schedule stores a defensive copy", "asil": "ASIL-B", "tags": ["master", "copy"]}, {"id": "REQ-MASTER-013", "title": "run continues after per-slot errors", "asil": "ASIL-B", "tags": ["master", "resilience"]}, diff --git a/src/ldf/parser.cpp b/src/ldf/parser.cpp index 3a97471..df6fda9 100644 --- a/src/ldf/parser.cpp +++ b/src/ldf/parser.cpp @@ -193,6 +193,16 @@ struct Parser { Signal sig; sig.name = name; try { sig.bit_width = static_cast(parse_int(parts[0])); } catch (...) {} + // Clamp bit_width to [0, 64] at parse time so DB::decode()'s + // extraction loop (`val |= uint64_t(1) << i` for i in + // [0, bit_width)) can never shift by >= 64, which is undefined + // behaviour, regardless of how large a malformed/adversarial LDF + // declares the signal width or how large the payload is. This is + // defence-in-depth: the loop's own byte_idx >= data.size() break + // happens to bound i for realistic (<=8-byte) frames today, but + // that is incidental, not a guarantee. + if (sig.bit_width < 0) sig.bit_width = 0; + else if (sig.bit_width > 64) sig.bit_width = 64; try { sig.init_value = parse_uint(parts[1]); } catch (...) {} sig.publisher = trim(parts[2]); for (std::size_t i = 3; i < parts.size(); ++i) { diff --git a/src/lin.cpp b/src/lin.cpp index 36418c5..6b4c160 100644 --- a/src/lin.cpp +++ b/src/lin.cpp @@ -178,6 +178,14 @@ class LinAdapter : public relay::INode { } catch (const ErrInvalidFrame&) { return relay::make_error_code(relay::Errc::payload_too_large); } + // Honour the frame's checksum type when registering the response: + // classic-configured frames (and all diagnostic frames 0x3C/0x3D, which + // MUST use the classic checksum per LIN 2.x §2.3.1.5 / RELAY §15.3) go + // through publish_classic(); otherwise the default publish() would + // silently upgrade every bridged frame to the enhanced checksum. + if (f.checksum_type == ChecksumType::Classic || + f.id == kLINDiagRequestID || f.id == kLINDiagResponseID) + return bus_->publish_classic(f.id, std::move(f.data)); return bus_->publish(f.id, std::move(f.data)); } diff --git a/src/master/node.cpp b/src/master/node.cpp index 97905d1..1fc0982 100644 --- a/src/master/node.cpp +++ b/src/master/node.cpp @@ -51,8 +51,11 @@ std::pair Node::send_header(uint8_t id) { // fusa:req REQ-MASTER-003 REQ-MASTER-004 REQ-MASTER-005 REQ-MASTER-006 // fusa:req REQ-MASTER-007 REQ-MASTER-008 REQ-MASTER-009 REQ-MASTER-013 std::error_code Node::run(const std::atomic& stop) { + // RELAY §8.3: an empty schedule table is valid and disables scheduled + // transmission — treat it as a no-op success, not an error, so callers + // don't have to special-case "no schedule configured" as a failure. if (schedule_.empty()) - return lin::make_error_code(lin::Errc::invalid_frame); + return {}; while (!stop.load()) { for (const auto& slot : schedule_) { diff --git a/src/virtual/bus.cpp b/src/virtual/bus.cpp index 91e8f7c..e9b8478 100644 --- a/src/virtual/bus.cpp +++ b/src/virtual/bus.cpp @@ -45,6 +45,13 @@ std::error_code Bus::do_publish(uint8_t id, std::vector data, ChecksumT if (data.empty()) { responses_.erase(id); } else { + // Diagnostic frames (0x3C master request, 0x3D slave response) MUST use + // the classic checksum regardless of the caller's requested type + // (LIN 2.x §2.3.1.5 / RELAY §15.3), so force it here — otherwise the + // default publish() path (which requests Enhanced) would emit a + // spec-violating enhanced-checksum diagnostic frame. + if (id == kLINDiagRequestID || id == kLINDiagResponseID) + ct = ChecksumType::Classic; // defensive copy already done by value parameter responses_[id] = ResponseEntry{std::move(data), ct}; } diff --git a/tests/test_ldf.cpp b/tests/test_ldf.cpp index 4b9e0ae..d94599d 100644 --- a/tests/test_ldf.cpp +++ b/tests/test_ldf.cpp @@ -132,6 +132,54 @@ TEST_CASE("Decode uses LSB-first Intel byte order", "[ldf][REQ-LDF-009]") { CHECK(decoded.at("MotorSpeed") == 0x42); } +TEST_CASE("parse clamps malformed/adversarial signal bit_width to [0,64]", "[ldf][REQ-LDF-007]") { + // Regression test for cpp-LIN-A2: DB::decode()'s bit-extraction loop does + // `val |= uint64_t(1) << i` for i in [0, bit_width). Without clamping + // bit_width at parse time, a malformed/adversarial LDF declaring an + // absurd bit_width (here 999999999, far beyond any real LIN signal) + // combined with a large-enough payload would shift by >= 64, which is + // undefined behaviour. bit_width must be bounded to <= 64 regardless of + // payload size, not merely bounded incidentally by the loop's own + // byte_idx >= data.size() break. + static const char* kMalformedLDF = R"( +LIN_description_file ; +LIN_protocol_version = "2.1" ; +LIN_language_version = "2.1" ; +LIN_speed = 19.2 kbps ; + +Nodes { + Master: BCM, 1 ms, 0.1 ms ; + Slaves: MotorControl ; +} + +Signals { + HugeSignal : 999999999, 0, MotorControl, BCM ; +} + +Frames { + HugeFrame : 0x10, MotorControl, 8 { + HugeSignal, 0 ; + } +} +)"; + std::istringstream ss(kMalformedLDF); + auto db = parse(ss); + REQUIRE(db != nullptr); + + auto sig = db->signal("HugeSignal"); + REQUIRE(sig != nullptr); + CHECK(sig->bit_width <= 64); + CHECK(sig->bit_width >= 0); + + // Decode with an oversized payload (well beyond a real LIN frame's 8-byte + // max) so that, absent the clamp, the loop would run i up to + // bit_width - 1 (>> 63) before byte_idx ever caught up — exercising the + // exact UB path. With the clamp in place this must complete safely. + std::vector huge_data(64, 0xFF); + auto decoded = db->decode(0x10, huge_data); + REQUIRE(decoded.count("HugeSignal") == 1); +} + TEST_CASE("Decode returns empty for unknown frame ID", "[ldf][REQ-LDF-010]") { std::istringstream ss(kSampleLDF); auto db = parse(ss); diff --git a/tests/test_master.cpp b/tests/test_master.cpp index df4471f..dd4bbe1 100644 --- a/tests/test_master.cpp +++ b/tests/test_master.cpp @@ -69,12 +69,15 @@ TEST_CASE("send_header delegates to bus", "[master][REQ-MASTER-002]") { (void)bus->close(); } -TEST_CASE("run returns error for empty schedule", "[master][REQ-MASTER-009]") { +TEST_CASE("run is a no-op success for empty schedule", "[master][REQ-MASTER-009]") { + // RELAY §8.3: an empty schedule table is valid and disables scheduled + // transmission, so run() must return success (not invalid_frame) and + // simply return immediately without touching the bus. auto bus = Bus::create(); Node node(bus); - std::atomic stop{true}; + std::atomic stop{false}; auto err = node.run(stop); - CHECK(err); // empty schedule + CHECK_FALSE(err); (void)bus->close(); } diff --git a/tests/test_relay_adapter.cpp b/tests/test_relay_adapter.cpp index a7be2dd..a3670df 100644 --- a/tests/test_relay_adapter.cpp +++ b/tests/test_relay_adapter.cpp @@ -39,6 +39,54 @@ TEST_CASE("adapt: send publishes payload to bus", "[adapter][REQ-ADAPT-002]") { (void)bus->close(); } +TEST_CASE("adapt: send preserves classic checksum type across the bridge", "[adapter][REQ-ADAPT-002][REQ-LIN-011][REQ-LIN-012]") { + // Regression test for cpp-LIN-02: a bridged message explicitly tagged + // lin.checksum_type=classic must be re-emitted on the virtual bus with + // the classic checksum, not silently upgraded to enhanced. + auto bus = Bus::create(); + auto node = adapt(bus); + + relay::Message msg; + msg.protocol = relay::Protocol::LIN; + msg.id = "16"; + msg.payload = {0xAA, 0xBB}; + msg.meta["lin.checksum_type"] = "classic"; + auto err = node->send(msg); + REQUIRE_FALSE(err); + + auto [f, ferr] = bus->send_header(0x10); + REQUIRE_FALSE(ferr); + CHECK(f.checksum_type == ChecksumType::Classic); + uint8_t pid = protect_id(0x10); + CHECK(f.checksum == calc_checksum(pid, f.data, ChecksumType::Classic)); + (void)bus->close(); +} + +TEST_CASE("adapt: send forces classic checksum for diagnostic frame IDs even when tagged enhanced", "[adapter][REQ-ADAPT-002][REQ-LIN-011][REQ-LIN-012]") { + // Regression test for cpp-LIN-01/02: diagnostic frames 0x3C/0x3D MUST + // always use the classic checksum (LIN 2.2A §2.3.1.5), even if the + // bridged message is (incorrectly) tagged lin.checksum_type=enhanced. + // This exercises the ID-based override independently of the + // checksum_type-based one. + auto bus = Bus::create(); + auto node = adapt(bus); + + relay::Message msg; + msg.protocol = relay::Protocol::LIN; + msg.id = "60"; // 0x3C + msg.payload = {0x01, 0x02}; + msg.meta["lin.checksum_type"] = "enhanced"; + auto err = node->send(msg); + REQUIRE_FALSE(err); + + auto [f, ferr] = bus->send_header(kLINDiagRequestID); + REQUIRE_FALSE(ferr); + CHECK(f.checksum_type == ChecksumType::Classic); + uint8_t pid = protect_id(kLINDiagRequestID); + CHECK(f.checksum == calc_checksum(pid, f.data, ChecksumType::Classic)); + (void)bus->close(); +} + TEST_CASE("adapt: send rejects out-of-range frame ID string", "[adapter][REQ-ADAPT-003]") { auto bus = Bus::create(); auto node = adapt(bus); diff --git a/tests/test_virtual.cpp b/tests/test_virtual.cpp index 97ec4d8..f76eeab 100644 --- a/tests/test_virtual.cpp +++ b/tests/test_virtual.cpp @@ -51,6 +51,29 @@ TEST_CASE("publish_classic uses classic checksum", "[virtual][REQ-VIRT-007]") { bus->close(); } +TEST_CASE("publish() on diagnostic IDs forces classic checksum", "[virtual][REQ-VIRT-002][REQ-VIRT-007]") { + // LIN 2.2A §2.3.1.5: frame IDs 0x3C (master request) and 0x3D (slave + // response) MUST always use the classic checksum, even when published + // through the default publish() entry point (which normally requests + // Enhanced). Regression test for cpp-LIN-01. + auto bus = Bus::create(); + REQUIRE_FALSE(bus->publish(kLINDiagRequestID, {0x01, 0x02})); + REQUIRE_FALSE(bus->publish(kLINDiagResponseID, {0x03, 0x04})); + + auto [req, req_err] = bus->send_header(kLINDiagRequestID); + REQUIRE_FALSE(req_err); + CHECK(req.checksum_type == ChecksumType::Classic); + uint8_t req_pid = protect_id(kLINDiagRequestID); + CHECK(req.checksum == calc_checksum(req_pid, req.data, ChecksumType::Classic)); + + auto [resp, resp_err] = bus->send_header(kLINDiagResponseID); + REQUIRE_FALSE(resp_err); + CHECK(resp.checksum_type == ChecksumType::Classic); + uint8_t resp_pid = protect_id(kLINDiagResponseID); + CHECK(resp.checksum == calc_checksum(resp_pid, resp.data, ChecksumType::Classic)); + bus->close(); +} + TEST_CASE("send_header broadcasts to subscribers", "[virtual][REQ-VIRT-008]") { auto bus = Bus::create(); auto [ch, err] = bus->subscribe({}, {});