From c0ec46c8a9659c00c97f54fc1c9fe6220d53c6d9 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Wed, 2 Sep 2026 23:34:21 +0200 Subject: [PATCH] fix(pj_plugins): validate a spliced GridMap right after attaching its bytes (SDK 0.26.1) deserializeGridMap accepts a header-only wire (the functional-v2 splice form) and leaves the data-length check to validateGridMap(). Both host splice attachment paths, the functional parser handle and the parser-module runtime, now run that check as soon as the bytes are attached, so a spliced GridMap whose bytes cannot cover its declared cells is rejected as a contract violation instead of reaching consumers. Host-side only: no header layout, ABI or wire change. Tests cover both paths with a two-column grid spliced with a single byte. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 10 ++++++++++ VERSION | 2 +- docs/builtin_type.md | 6 +++--- .../pj_base/builtin/grid_map_codec.hpp | 4 ++-- .../pj_plugins/host/message_parser_handle.hpp | 9 +++++++++ pj_plugins/src/parser_module_runtime.cpp | 8 ++++++++ ...ssage_parser_functional_extension_test.cpp | 20 +++++++++++++++++++ .../tests/native_parser_module_fixture.cpp | 13 +++++++++--- .../tests/native_parser_module_fixture.hpp | 3 ++- .../tests/parser_module_runtime_test.cpp | 9 +++++++++ 10 files changed, 74 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 54127ce7..7d0a7f72 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,16 @@ All notable changes to `plotjuggler_sdk` are recorded here. Versioning policy is in [`CLAUDE.md`](./CLAUDE.md) → "Release Versioning". +## [0.26.1] + +### Fix: hosts validate a spliced `GridMap` right after attaching its bytes (PATCH) + +`deserializeGridMap` accepts a header-only wire (the functional-v2 splice form) and leaves the +data-length check to `validateGridMap()`. Both host splice-attachment paths (the functional +parser handle and the parser-module runtime) now run that check as soon as the bytes are attached, +so a spliced GridMap whose bytes cannot cover its declared cells is rejected as a contract +violation instead of reaching consumers. Host-side only; no header layout, ABI or wire change. + ## [0.26.0] ### Feature: `GridMap` canonical builtin object (MINOR) diff --git a/VERSION b/VERSION index 4e8f395f..30f6cf8d 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.26.0 +0.26.1 diff --git a/docs/builtin_type.md b/docs/builtin_type.md index 2c10395b..14343d67 100644 --- a/docs/builtin_type.md +++ b/docs/builtin_type.md @@ -666,9 +666,9 @@ layout the cell math could not index safely (a field with an unknown datatype or zero count, a field reaching past `cell_stride`, a zero stride with cells declared, a row shorter than its columns, `data` shorter than `row_count * row_stride`); a wire that carries no `data` is the functional-v2 -splice form and decodes with an empty span, so hosts that attach spliced bytes -and consumers that index cells run `validateGridMap()` once the bytes are in -place. +splice form and decodes with an empty span; the host runs `validateGridMap()` +right after attaching the spliced bytes, and consumers that index cells run it +again defensively. ## Conversion Examples diff --git a/pj_base/include/pj_base/builtin/grid_map_codec.hpp b/pj_base/include/pj_base/builtin/grid_map_codec.hpp index 1fbfe713..fdcf8b2e 100644 --- a/pj_base/include/pj_base/builtin/grid_map_codec.hpp +++ b/pj_base/include/pj_base/builtin/grid_map_codec.hpp @@ -29,8 +29,8 @@ inline constexpr std::string_view kSchemaGridMap = "PJ.GridMap"; /// Full layout check for a grid whose bytes are in place: every field has a /// known datatype, a non-zero count and ends within `cell_stride`; with cells /// declared, both strides are non-zero, a row holds its columns, and `data` -/// covers `row_count * row_stride`. Consumers that index cells, and hosts that -/// attach spliced bytes, call this before trusting the layout. +/// covers `row_count * row_stride`. Hosts call it right after attaching spliced +/// bytes; consumers that index cells call it again before trusting the layout. [[nodiscard]] Expected validateGridMap(const sdk::GridMap& grid); } // namespace PJ diff --git a/pj_plugins/include/pj_plugins/host/message_parser_handle.hpp b/pj_plugins/include/pj_plugins/host/message_parser_handle.hpp index 1d737773..02c92006 100644 --- a/pj_plugins/include/pj_plugins/host/message_parser_handle.hpp +++ b/pj_plugins/include/pj_plugins/host/message_parser_handle.hpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -504,6 +505,14 @@ class MessageParserHandle { fillContractViolation(out_error, "functional parser splice object type is not materializable"); return false; } + // GridMap decodes header-only for splices; the data-length check waits + // until the bytes exist, which is now. + if (type == sdk::BuiltinObjectType::kGridMap) { + if (auto valid = validateGridMap(*std::any_cast(&*object)); !valid) { + fillContractViolation(out_error, "spliced GridMap layout is invalid: " + valid.error()); + return false; + } + } state->record = sdk::ObjectRecord{ .ts = has_timestamp ? std::optional(timestamp_ns) : std::nullopt, .object = std::move(*object), diff --git a/pj_plugins/src/parser_module_runtime.cpp b/pj_plugins/src/parser_module_runtime.cpp index a9ac4585..09ff1f0d 100644 --- a/pj_plugins/src/parser_module_runtime.cpp +++ b/pj_plugins/src/parser_module_runtime.cpp @@ -16,6 +16,7 @@ #include "detail/native_parser_module_state.hpp" #include "pj_base/builtin/builtin_object_codec.hpp" +#include "pj_base/builtin/grid_map_codec.hpp" #include "pj_base/builtin_object_abi.h" #include "pj_base/span.hpp" @@ -146,6 +147,13 @@ Expected ownObjectOutput( if (!attached) { return unexpected("output splice could not be attached to its canonical object"); } + // GridMap decodes header-only for splices; the data-length check waits + // until the bytes exist, which is now. + if (type == sdk::BuiltinObjectType::kGridMap) { + if (auto valid = validateGridMap(*std::any_cast(&owned.object)); !valid) { + return unexpected("output spliced GridMap layout is invalid: " + valid.error()); + } + } owned.splice = ParserModuleObjectSplice{ .field_number = object.splice->field_number, .input_offset = object.splice->input_offset, diff --git a/pj_plugins/tests/message_parser_functional_extension_test.cpp b/pj_plugins/tests/message_parser_functional_extension_test.cpp index d3bd6987..a71560df 100644 --- a/pj_plugins/tests/message_parser_functional_extension_test.cpp +++ b/pj_plugins/tests/message_parser_functional_extension_test.cpp @@ -191,6 +191,17 @@ bool emitSplicedGridMapV2( PJ_bytes_view_t{kGridMapWire.data(), kGridMapWire.size()}, 10, 1, 2, out_error); } +// Same header as emitSplicedGridMapV2 but two columns (row_stride 2) and a +// one-byte splice: the attached bytes cannot cover the declared cells. +bool emitSplicedGridMapTooShortV2( + void*, int64_t, PJ_payload_t, const PJ_parser_object_sink_v2_t* sink, PJ_error_t* out_error) noexcept { + static constexpr std::array kGridMapWire{0x28, 0x02, 0x30, 0x01, 0x38, 0x01, 0x40, 0x02}; + return sink != nullptr && sink->accept_object_spliced != nullptr && + sink->accept_object_spliced( + sink->ctx, true, 90, PJ_BUILTIN_OBJECT_TYPE_GRID_MAP, + PJ_bytes_view_t{kGridMapWire.data(), kGridMapWire.size()}, 10, 1, 1, out_error); +} + bool emitMismatchedImageV2( void*, int64_t, PJ_payload_t, const PJ_parser_object_sink_v2_t* sink, PJ_error_t* out_error) noexcept { static constexpr std::array kImageWire{0x10, 0x01}; @@ -472,6 +483,15 @@ TEST(MessageParserFunctionalExtension, HostV2PathReconstructsGridMapSplice) { EXPECT_TRUE(PJ::validateGridMap(*grid).has_value()); } +TEST(MessageParserFunctionalExtension, HostV2PathRejectsSplicedGridMapWhoseBytesDoNotCoverTheCells) { + PJ::MessageParserHandle handle(adversarialV2Vtable()); + ASSERT_TRUE(handle.bindSchema("example/GridMap", {})); + const std::array payload{10, 20, 30, 40}; + auto record = handle.parseObjectFunctional(0, PJ::Span(payload)); + ASSERT_FALSE(record.has_value()); + EXPECT_NE(record.error().find("GridMap"), std::string::npos) << record.error(); +} + TEST(MessageParserFunctionalExtension, HostRejectsObjectTypeThatDiffersFromBindingClassification) { PJ::MessageParserHandle handle(adversarialV2Vtable()); ASSERT_TRUE(handle.bindSchema("example/PointCloud", {})); diff --git a/pj_plugins/tests/native_parser_module_fixture.cpp b/pj_plugins/tests/native_parser_module_fixture.cpp index 23389008..2e36d746 100644 --- a/pj_plugins/tests/native_parser_module_fixture.cpp +++ b/pj_plugins/tests/native_parser_module_fixture.cpp @@ -47,7 +47,8 @@ constexpr char kManifest[] = R"({ {"claim_id":"bad-token","encoding":"protobuf","type_name":"fixture.BadToken","routes":["scalar"],"priority":0}, {"claim_id":"route-mismatch","encoding":"protobuf","type_name":"fixture.RouteMismatch","routes":["scalar"],"priority":0}, {"claim_id":"type-mismatch","encoding":"protobuf","type_name":"fixture.TypeMismatch","routes":["object"],"object_type":"kPointCloud","priority":0}, - {"claim_id":"splice-grid-map","encoding":"protobuf","type_name":"fixture.SpliceGridMap","routes":["object"],"object_type":"kGridMap","priority":0} + {"claim_id":"splice-grid-map","encoding":"protobuf","type_name":"fixture.SpliceGridMap","routes":["object"],"object_type":"kGridMap","priority":0}, + {"claim_id":"splice-grid-map-short","encoding":"protobuf","type_name":"fixture.SpliceGridMapShort","routes":["object"],"object_type":"kGridMap","priority":0} ] })"; @@ -228,7 +229,8 @@ PJ_FIXTURE_EXPORT int32_t pj_module_parse( .wire = {}, }; break; - case kSpliceGridMap: { + case kSpliceGridMap: + case kSpliceGridMapShort: { PJ::sdk::GridMap grid; // header only: the two cell bytes arrive as the splice grid.column_count = 2; grid.row_count = 1; @@ -239,7 +241,12 @@ PJ_FIXTURE_EXPORT int32_t pj_module_parse( wire = PJ::serializeGridMap(grid); descriptor = PJ::parser_module::ObjectOutputV1{ .object_type = PJ_BUILTIN_OBJECT_TYPE_GRID_MAP, - .splice = PJ::parser_module::ObjectSpliceV1{.field_number = 10, .input_offset = 1, .input_length = 2}, + .splice = + PJ::parser_module::ObjectSpliceV1{ + .field_number = 10, + .input_offset = 1, + // The short variant attaches one byte for a two-byte row. + .input_length = instance->claim_index == kSpliceGridMapShort ? uint64_t{1} : uint64_t{2}}, .wire = wire, }; break; diff --git a/pj_plugins/tests/native_parser_module_fixture.hpp b/pj_plugins/tests/native_parser_module_fixture.hpp index 0827193b..51b18a4f 100644 --- a/pj_plugins/tests/native_parser_module_fixture.hpp +++ b/pj_plugins/tests/native_parser_module_fixture.hpp @@ -23,7 +23,8 @@ enum ClaimIndex : uint32_t { kRouteMismatch = 10, kTypeMismatch = 11, kSpliceGridMap = 12, - kClaimCount = 13, + kSpliceGridMapShort = 13, + kClaimCount = 14, }; } // namespace pj_fixture diff --git a/pj_plugins/tests/parser_module_runtime_test.cpp b/pj_plugins/tests/parser_module_runtime_test.cpp index 2a01a779..0afeec08 100644 --- a/pj_plugins/tests/parser_module_runtime_test.cpp +++ b/pj_plugins/tests/parser_module_runtime_test.cpp @@ -201,6 +201,15 @@ TEST(ParserModuleRuntime, AttachesGridMapSpliceToTheDecodedHeader) { EXPECT_TRUE(validateGridMap(*grid).has_value()); } +TEST(ParserModuleRuntime, RejectsGridMapSpliceWhoseBytesDoNotCoverTheCells) { + auto module = loadFixture(); + auto bound = createBound(module, kSpliceGridMapShort, parser_module::Route::kObject, PJ_BUILTIN_OBJECT_TYPE_GRID_MAP); + auto result = bound.parse(input()); + ASSERT_TRUE(result.has_value()) << result.error(); + EXPECT_EQ(result->fault, ParserModuleFaultKind::kContractViolation); + EXPECT_NE(result->message.find("GridMap"), std::string::npos) << result->message; +} + TEST(ParserModuleRuntime, StrikeTrackerQuarantinesReplaysAndThenDisables) { auto module = loadFixture(); const ParserModuleClaimKey key{"org.plotjuggler.test.native-module", "malformed"};