fix(codec): harden FlatBuffers reflection codec for automatic (de)serialization - #178
fix(codec): harden FlatBuffers reflection codec for automatic (de)serialization#17816bit-ykiko wants to merge 4 commits into
Conversation
…ialization
- Generalize the map-entry protocol: entries may be tuple-like, pair-like,
expose getKey()/getValue() (llvm::StringMap), or be plain two-field
aggregates. Encode/decode and runtime type_info all use the shared
accessors, and decode normalizes view-typed keys (string_view,
llvm::StringRef) into owning storage before insertion.
- Sort encoded map entries by the key's own ordering instead of its
stringified form. Multi-digit integer keys previously broke
map_view::find's binary search ("10" < "9"), and struct keys did not
compile at all; both now work and stay consistent with proxy lookups.
- Propagate encode failures out of nested tables: two_pass returns nullopt
on error instead of a zero offset that visit_struct silently swallowed,
and variant payload write failures no longer go unnoticed.
- Encode long double as double everywhere; the decoder always read 8 bytes
while the encoder wrote sizeof(long double), corrupting round-trips.
- Add verify_flatbuffer<T>: schema-driven deep verification that
bounds-checks every table, vector, string and scalar reachable through
T's layout. from_flatbuffer now runs it before decoding, making corrupt
or truncated buffers a clean error instead of out-of-bounds reads; it is
also exposed standalone for zero-copy table_view readers.
New regression suites cover scrambled integer/struct/string-map keys,
StringMap-style and aggregate-entry containers, truncation and bit-flip
fuzzing under ASan/UBSan, nested encode-failure propagation, and
long double round-trips.
…truction
- Treat trivially-copyable standard-layout aggregates as inline structs even
when they carry default member initializers; inline structs are memcpy'd,
so std::is_trivial was needlessly strict and demoted common structs like
range types to full tables.
- Value-initialize decode temporaries with T() instead of T{}: aggregate
copy-list-initialization rejects members whose default constructor is
explicit (e.g. llvm::DenseMap), which broke decoding maps of aggregates
holding such containers.
📝 WalkthroughWalkthroughAdds deep FlatBuffers verification before decode, propagates encoding failures with optional returns, broadens map-entry detection and comparison support, and adds map-protocol plus robustness tests. ChangesFlatBuffers Codec Hardening
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/unit/codec/fbs/flatbuffers_map_protocol_tests.cpp (1)
368-389: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the aggregate-entry decode assertion.
Unlike the sibling tests (u64, span_key, string_map), this test only checks
decoded.table.entries.size()after round-trip and never verifies that the decodedid/labelvalues actually match the inputs. Since this exercises the newly-added two-field-aggregate entry protocol, verifying content would catch a class of bugs (e.g., wrong field assigned to key vs. value) that size-only checks can't.♻️ Proposed strengthening
aggregate_entry_holder decoded; auto result = from_flatbuffer(std::span<const std::uint8_t>(*encoded), decoded); ASSERT_TRUE(result.has_value()); ASSERT_EQ(decoded.table.entries.size(), 3U); + for(const auto& e: decoded.table.entries) { + if(e.id == 12U) EXPECT_EQ(e.label, "twelve"); + else if(e.id == 3U) EXPECT_EQ(e.label, "three"); + else if(e.id == 101U) EXPECT_EQ(e.label, "hundred-one"); + else FAIL("unexpected decoded key"); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/codec/fbs/flatbuffers_map_protocol_tests.cpp` around lines 368 - 389, The aggregate_entry_container_encodes_as_map test only checks the decoded entry count, so strengthen the round-trip verification in aggregate_entry_holder decoding. After from_flatbuffer succeeds, inspect decoded.table.entries and assert that the decoded aggregate-entry fields match the original inputs for all three items, similar to the sibling map tests. Use the aggregate_entry_holder, to_flatbuffer, and from_flatbuffer paths to locate the test and verify both key/value content and the specific id/label field mapping, not just size.tests/unit/codec/fbs/flatbuffers_robustness_tests.cpp (1)
111-127: 🩺 Stability & Availability | 🔵 TrivialCrash-only assertions depend on sanitizer instrumentation.
verify_rejects_wrong_root_type_gracefullyandbitflipped_buffers_never_crashdeliberately discard the decode result and rely on comments like "the run must be clean under ASan" for their real signal. Please confirm this test binary is actually built/run with ASan (and ideally UBSan) in CI — otherwise these tests will pass even if an OOB read occurs without triggering a crash on this particular platform/allocator.Also applies to: 152-165
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/codec/fbs/flatbuffers_robustness_tests.cpp` around lines 111 - 127, The robustness tests `verify_rejects_wrong_root_type_gracefully` and `bitflipped_buffers_never_crash` only detect bad behavior when sanitizer instrumentation is enabled, so update the CI/test setup to build and run this codec test binary with ASan and ideally UBSan. Check the test target configuration for these Flatbuffers tests and ensure the sanitizer-enabled job covers the same executable so the discarded `from_flatbuffer` results still fail on out-of-bounds or UB.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@include/kota/support/ranges.h`:
- Around line 34-37: `map_entry_keyed_like` is probing `getKey()` and
`getValue()` on a non-const reference, but the encode paths use
`map_entry_key()` and `map_entry_value()` on a const entry, so the concept can
accept types that still fail later. Update the concept in `ranges.h` to require
the accessors on `const T&` instead of `T&`, keeping the detection aligned with
the actual usage in the map entry helpers.
---
Nitpick comments:
In `@tests/unit/codec/fbs/flatbuffers_map_protocol_tests.cpp`:
- Around line 368-389: The aggregate_entry_container_encodes_as_map test only
checks the decoded entry count, so strengthen the round-trip verification in
aggregate_entry_holder decoding. After from_flatbuffer succeeds, inspect
decoded.table.entries and assert that the decoded aggregate-entry fields match
the original inputs for all three items, similar to the sibling map tests. Use
the aggregate_entry_holder, to_flatbuffer, and from_flatbuffer paths to locate
the test and verify both key/value content and the specific id/label field
mapping, not just size.
In `@tests/unit/codec/fbs/flatbuffers_robustness_tests.cpp`:
- Around line 111-127: The robustness tests
`verify_rejects_wrong_root_type_gracefully` and `bitflipped_buffers_never_crash`
only detect bad behavior when sanitizer instrumentation is enabled, so update
the CI/test setup to build and run this codec test binary with ASan and ideally
UBSan. Check the test target configuration for these Flatbuffers tests and
ensure the sanitizer-enabled job covers the same executable so the discarded
`from_flatbuffer` results still fail on out-of-bounds or UB.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8bf32c5d-e20b-4176-bc8c-1f5f29718215
📒 Files selected for processing (9)
include/kota/codec/fbs/decode.hinclude/kota/codec/fbs/encode.hinclude/kota/codec/fbs/type.hinclude/kota/codec/visit/decode.hinclude/kota/codec/visit/encode.hinclude/kota/meta/type_info.hinclude/kota/support/ranges.htests/unit/codec/fbs/flatbuffers_map_protocol_tests.cpptests/unit/codec/fbs/flatbuffers_robustness_tests.cpp
| concept map_entry_keyed_like = requires(T& t) { | ||
| t.getKey(); | ||
| t.getValue(); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files and inspect the exact lines around the concept and its uses.
git ls-files include/kota/support/ranges.h include/kota/support/visit/encode.h
echo "---- ranges.h ----"
cat -n include/kota/support/ranges.h | sed -n '1,140p'
echo "---- encode.h matches ----"
rg -n "map_entry_key|map_entry_value|const auto& entry|encode_sorted_map|StringMapEntry" include/kota/support/visit/encode.h -A3 -B3
echo "---- StringMapEntry accessor signatures from repository references ----"
rg -n "class StringMapEntry|getKey\\(|getValue\\(" include -A2 -B2Repository: clice-io/kotatsu
Length of output: 6101
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== call sites =="
rg -n "map_entry_key\\(|map_entry_value\\(|map_entry_keyed_like|StringMapEntry|getKey\\(|getValue\\(" . -g '!*.git' -g '!build' -g '!dist' -g '!node_modules' -A3 -B3
echo
echo "== files that look relevant =="
fd -t f 'encode.h|ranges.h|StringMapEntry' .
echo
echo "== inspect any encode-related headers under include/kota =="
fd -t f . include/kota | rg 'encode|visit|range|map'
echo
echo "== search for const-keyed usage patterns =="
rg -n "const auto& entry|auto& entry|for \\(.*entry" include src test -A2 -B2Repository: clice-io/kotatsu
Length of output: 13799
Probe keyed entries with const T&
map_entry_keyed_like checks getKey()/getValue() on T&, but map_entry_key() and map_entry_value() are called on const auto& entry in the encode paths. A type with only non-const accessors would satisfy the concept and still fail at the call site. Probe with const T& so the detection matches the actual use.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@include/kota/support/ranges.h` around lines 34 - 37, `map_entry_keyed_like`
is probing `getKey()` and `getValue()` on a non-const reference, but the encode
paths use `map_entry_key()` and `map_entry_value()` on a const entry, so the
concept can accept types that still fail later. Update the concept in `ranges.h`
to require the accessors on `const T&` instead of `T&`, keeping the detection
aligned with the actual usage in the map entry helpers.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4620e9bd8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if constexpr(proxy_detail::is_scalar_v<T0> || std::same_as<T0, std::byte>) { | ||
| using storage_t = proxy_detail::scalar_storage_t<T0>; | ||
| return tbl->template VerifyField<storage_t>(v, slot, alignof(storage_t)); |
There was a problem hiding this comment.
When a caller uses a config with enum_repr::String, encode_value writes enum fields as FlatBuffer strings, but this verifier still reaches the scalar branch because proxy_detail::is_scalar_v includes enums. Since from_flatbuffer<ThatConfig> now always runs verify_flatbuffer first, valid buffers containing string-encoded enum fields are rejected with buffer verification failed before decode can read them; the verifier needs to check Config::enum_repr for enum types before validating them as scalar storage.
Useful? React with 👍 / 👎.
| if constexpr(meta::annotated_type<T0>) { | ||
| return verify_root<std::remove_cvref_t<typename T0::annotated_type>, Config>(v, root); |
There was a problem hiding this comment.
For a top-level annotated value, the encoder/decoder honor behavior annotations such as enum_string or behavior::with, but this root verifier strips the annotation and validates the raw inner type instead. A valid buffer for meta::enum_string<MyEnum> or a root behavior::with adapter that encodes to a string will now be rejected by from_flatbuffer before decoding; this should mirror the annotation handling used by verify_field rather than unconditionally unwrapping to annotated_type.
Useful? React with 👍 / 👎.
| verifier_t verifier(data, buf.size()); | ||
| // GetRoot only reads the root uoffset (guarded by the size check above); | ||
| // verify_root's VerifyTableStart bounds-checks the table it points at. | ||
| const auto* root = ::flatbuffers::GetRoot<Table>(data); |
There was a problem hiding this comment.
If a corrupt buffer preserves the EVTO identifier but changes the first uoffset, this constructs the root pointer before the verifier has checked that the offset is non-zero and inside the buffer. In particular, a zero root offset can be treated as an empty table and decoded as defaults instead of failing verification, while a huge offset forms an out-of-buffer pointer before validation; validate the root offset before calling GetRoot.
Useful? React with 👍 / 👎.
| std::vector<std::string_view> keys; | ||
| keys.reserve(offsets.size()); | ||
| for(const auto& entry: m) { | ||
| keys.emplace_back(std::string_view(kota::detail::map_entry_key(entry))); |
There was a problem hiding this comment.
For a keyed-entry container whose getKey() returns an owning std::string by value, this stores a string_view to that temporary and then sorts after the temporary has been destroyed. That makes wire ordering undefined or incorrect for those map-like containers; store an owning key here unless the accessor result is known to reference container-owned storage.
Useful? React with 👍 / 👎.
| return false; | ||
| } | ||
| for(uoffset_t i = 0; i < vec->size(); ++i) { | ||
| const auto* entry = vec->template GetAs<Table>(i); |
There was a problem hiding this comment.
After VerifyVector(vec), only the vector object and offset array are known to be in bounds; the table offsets stored in the vector have not been validated. With a malformed map/vector entry containing offset 0 or an out-of-range offset, GetAs<Table> can be treated as a bogus table and the buffer may verify and decode default entries instead of being rejected, so validate each element offset before verify_elem_table and apply the same fix to the table-vector loops below.
Useful? React with 👍 / 👎.
| std::is_trivially_copyable_v<T> && | ||
| std::is_default_constructible_v<T> && | ||
| std::is_standard_layout_v<T> && fields_supported(); |
There was a problem hiding this comment.
With this broadened inline-struct predicate, a reflected struct that contains a std::optional<int> can now be classified as an inline FlatBuffers struct because the field check strips optional and common standard libraries make optional<int> trivially copyable. That causes the optional's ABI-specific object representation to be memcpy'd into the buffer and later accepted by the verifier as just bytes, so cross-version/corrupt buffers can produce invalid optional state; structs with nullable fields should stay table-shaped.
Useful? React with 👍 / 👎.
| using key_t = std::conditional_t<meta::str_like<raw_key_t> && | ||
| !std::same_as<raw_key_t, std::string>, | ||
| std::string, | ||
| raw_key_t>; |
There was a problem hiding this comment.
For output maps whose actual key type is a view such as std::string_view, this aliases key_t to std::string, then insert_map_entry can satisfy insert_or_assign by converting that temporary string into a string_view key. The temporary is destroyed immediately after insertion, leaving the decoded map with dangling keys; only containers that copy keys into owned storage should use this normalization path.
Useful? React with 👍 / 👎.
… operator< Both the encoder's entry sort and map_view's binary search now go through meta::lt: keys that define operator< keep their own ordering, and keys without any comparison operator get the reflection-synthesized field-by-field order. The lookup constraints move from totally_ordered_with to the matching meta::synthesized_lt_with concept, so comparison-free aggregate keys become searchable instead of silently unsorted.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/codec/fbs/flatbuffers_map_protocol_tests.cpp (1)
377-381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen decode assertions for the two newest map-entry protocols.
Both
comparison_free_struct_keys_sort_and_lookupandaggregate_entry_container_encodes_as_maponly checkdecoded.table.entries.size()after decode, unlike the sibling tests (u64_keys_survive_round_trip_and_lookup,inline_struct_keys_sort_and_lookup) which verify every decoded key/value pair matches the input. Since these two cases exercise the newest protocol paths in this PR (comparison-free synthesized ordering, and two-field aggregate entries), verifying only the count leaves a gap where a key/value swap or corruption on decode wouldn't be caught.♻️ Suggested addition for bare_key_holder decode check
bare_key_holder decoded; auto result = from_flatbuffer(std::span<const std::uint8_t>(*encoded), decoded); ASSERT_TRUE(result.has_value()); EXPECT_EQ(decoded.table.entries.size(), 5U); + for(const auto& [key, value]: input.table.entries) { + bool found = false; + for(const auto& [dk, dv]: decoded.table.entries) { + if(dk == key) { + EXPECT_EQ(dv, value); + found = true; + } + } + EXPECT_TRUE(found); + } }Similar per-entry validation (e.g., via
decoded.table.find(id)) would help foraggregate_entry_container_encodes_as_mapas well.Also applies to: 438-442
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/codec/fbs/flatbuffers_map_protocol_tests.cpp` around lines 377 - 381, The decode checks in comparison_free_struct_keys_sort_and_lookup and aggregate_entry_container_encodes_as_map only verify the number of decoded entries, so strengthen them by validating each expected key/value pair after from_flatbuffer succeeds. Use the existing decoded.table accessors seen in sibling tests like u64_keys_survive_round_trip_and_lookup and inline_struct_keys_sort_and_lookup, and add per-entry lookups/assertions with decoded.table.find(...) (or equivalent) to confirm the decoded map contents exactly match the input for these newer protocol paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/codec/fbs/flatbuffers_map_protocol_tests.cpp`:
- Around line 377-381: The decode checks in
comparison_free_struct_keys_sort_and_lookup and
aggregate_entry_container_encodes_as_map only verify the number of decoded
entries, so strengthen them by validating each expected key/value pair after
from_flatbuffer succeeds. Use the existing decoded.table accessors seen in
sibling tests like u64_keys_survive_round_trip_and_lookup and
inline_struct_keys_sort_and_lookup, and add per-entry lookups/assertions with
decoded.table.find(...) (or equivalent) to confirm the decoded map contents
exactly match the input for these newer protocol paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb146273-7269-496a-8c14-68845961f27c
📒 Files selected for processing (4)
include/kota/codec/fbs/encode.hinclude/kota/codec/fbs/proxy.hinclude/kota/meta/compare.htests/unit/codec/fbs/flatbuffers_map_protocol_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- include/kota/codec/fbs/encode.h
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5882279aa5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return (verify_field<std::remove_cv_t<typename type_list_element_t<Is, slots>::raw_type>, | ||
| Config>( |
There was a problem hiding this comment.
For struct fields whose schema slot changes the wire layout, such as annotation<int, behavior::with<Adapter>> where Adapter::wire_type is std::string, this verifies the raw field type instead of the slot's computed wire_type. In that scenario a corrupted string offset only has to pass VerifyField<int> and is never checked with VerifyString, so from_flatbuffer can still dereference an unchecked offset when the decoder later calls visit_str; use the virtual-schema slot wire type/attrs for verification rather than raw_type.
Useful? React with 👍 / 👎.
| for(const auto& entry: m) { | ||
| keys.emplace_back(std::string_view(kota::detail::map_entry_key(entry))); | ||
| } |
There was a problem hiding this comment.
When the encoded map-like container is only an input_range (which the codec concepts accept), body(coll) has already iterated it once to build the entry offsets, and this second loop may be empty or produce a different sequence of keys. In that case the offsets are left unsorted or sorted against the wrong keys, so map_view::find's binary search can miss entries from valid buffers; collect each key in the same visit_entry pass as its offset instead of re-iterating m.
Useful? React with 👍 / 👎.
| } else if constexpr(meta::reflectable_class<T0>) { | ||
| return verify_child_table(v, tbl, slot, [&](const Table* c) { | ||
| return verify_struct_slots<T0, Config>(v, c); |
There was a problem hiding this comment.
If a reflectable class also has a custom serialize_visit/deserialize_visit specialization without a wire_type, the codec dispatches to that opaque visitor but the verifier falls through here and treats the value as a reflected table. A valid custom encoding such as a string or byte vector is then rejected before decode (or verified against the wrong shape), despite the new verifier comments saying such opaque subtrees are skipped; detect these custom visitors and skip them unless a wire_type is provided.
Useful? React with 👍 / 👎.
…rom_wire A serialize_visit specialization already declares wire_type for schema-aware backends; it can now optionally provide to_wire/from_wire instead of visit, and a single specialization then serves both encoding and decoding on every backend: encode_value forwards through to_wire, decode_value reads the declared wire_type and converts back through from_wire. This removes the need for a paired deserialize_visit specialization when an adapter is a pure value conversion.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
include/kota/codec/visit/decode.h (1)
732-742: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce repetition of the 4-arg
serialize_visitspecialization.
serialize_visit<Vis, V, Config, void>is spelled out three times in this branch. Nearbybehavior::with/behavior::asbranches (e.g. lines 769-778) introduce a local alias for the analogous adapter type. Doing the same here would match the file's existing style and avoid drift if one occurrence is edited later.♻️ Proposed refactor
- } else if constexpr(requires { - serialize_visit<Vis, V, Config, void>::from_wire( - std::declval< - typename serialize_visit<Vis, V, Config, void>::wire_type>()); - }) { - // Value-mode serialize_visit specialization: decode the declared - // wire_type, then convert back through from_wire. - auto wire = typename serialize_visit<Vis, V, Config, void>::wire_type(); - KOTA_CODEC_TRY(decode_value<Config>(vis, wire)); - out = serialize_visit<Vis, V, Config, void>::from_wire(std::move(wire)); - return true; + } else if constexpr(requires { + typename serialize_visit<Vis, V, Config, void>::wire_type; + serialize_visit<Vis, V, Config, void>::from_wire( + std::declval< + typename serialize_visit<Vis, V, Config, void>::wire_type>()); + }) { + using svisit = serialize_visit<Vis, V, Config, void>; + // Value-mode serialize_visit specialization: decode the declared + // wire_type, then convert back through from_wire. + auto wire = typename svisit::wire_type(); + KOTA_CODEC_TRY(decode_value<Config>(vis, wire)); + out = svisit::from_wire(std::move(wire)); + return true;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/kota/codec/visit/decode.h` around lines 732 - 742, Introduce a local alias for the 4-arg serialize_visit specialization in this decode branch, similar to the nearby behavior::with and behavior::as branches, so the repeated serialize_visit<Vis, V, Config, void> type is spelled once and reused. Update the requires check, wire_type construction, and from_wire call in the decode_value path to use that alias, keeping the existing decode logic unchanged.include/kota/codec/visit/encode.h (1)
175-179: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider asserting
to_wire's return maps to the same wire kind aswire_type.This branch forwards whatever
to_wire(value)returns straight intoencode_value, independent of the declaredwire_type. Decoding, however, always decodes intowire_typeand then callsfrom_wire. Round-tripping only works ifto_wire's return type serializes identically towire_type(e.g.std::string_viewvsstd::stringboth route throughvisit_str, so today's test case is fine) — but nothing stops a future specialization from returning a type in a differentmeta::type_kindcategory thanwire_type, which would silently produce mismatched wire formats only surfacing as a runtime round-trip failure.🛡️ Proposed guard
} else if constexpr(requires(const V& val) { serialize_visit<Vis, V, Config>::to_wire(val); }) { + static_assert(meta::kind_of<std::remove_cvref_t<decltype(serialize_visit<Vis, V, Config>::to_wire( + std::declval<const V&>()))>>() == + meta::kind_of<typename serialize_visit<Vis, V, Config>::wire_type>(), + "to_wire() return type must serialize as the same wire kind as wire_type"); // Value-mode specialization: `wire_type` declares the on-wire layout,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/kota/codec/visit/encode.h` around lines 175 - 179, The value-mode branch in encode_value is forwarding serialize_visit<Vis, V, Config>::to_wire(value) without verifying it matches the declared wire_type, which can create silent encode/decode mismatches. Add a compile-time guard in this path, using the serialize_visit specialization and wire_type, to assert that to_wire’s return type maps to the same meta::type_kind as wire_type before calling encode_value. Keep the check close to the requires branch in encode.h so future specializations cannot drift into a different wire category.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@include/kota/codec/visit/decode.h`:
- Around line 732-742: Introduce a local alias for the 4-arg serialize_visit
specialization in this decode branch, similar to the nearby behavior::with and
behavior::as branches, so the repeated serialize_visit<Vis, V, Config, void>
type is spelled once and reused. Update the requires check, wire_type
construction, and from_wire call in the decode_value path to use that alias,
keeping the existing decode logic unchanged.
In `@include/kota/codec/visit/encode.h`:
- Around line 175-179: The value-mode branch in encode_value is forwarding
serialize_visit<Vis, V, Config>::to_wire(value) without verifying it matches the
declared wire_type, which can create silent encode/decode mismatches. Add a
compile-time guard in this path, using the serialize_visit specialization and
wire_type, to assert that to_wire’s return type maps to the same meta::type_kind
as wire_type before calling encode_value. Keep the check close to the requires
branch in encode.h so future specializations cannot drift into a different wire
category.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55dff232-d9f5-43ba-9f52-90fc6c8ffe94
📒 Files selected for processing (3)
include/kota/codec/visit/decode.hinclude/kota/codec/visit/encode.htests/unit/codec/fbs/flatbuffers_wire_adapter_tests.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ff9bf1489
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if constexpr(requires(const V& val) { serialize_visit<Vis, V, Config>::to_wire(val); }) { | ||
| // Value-mode specialization: `wire_type` declares the on-wire layout, | ||
| // to_wire/from_wire convert. One specialization serves both | ||
| // directions (decode_value picks up from_wire). | ||
| return encode_value<Config>(vis, serialize_visit<Vis, V, Config>::to_wire(value)); |
There was a problem hiding this comment.
When this value-mode path is reached while FlatBuffers is encoding a sequence whose collector did not recognize the same wire_type (for example a serialize_visit enabled only for a user Config, while seq_encode_impl still probes default_config<>), vis is the table-element collector; recursively encoding a scalar/string wire value then hits that collector's no-op scalar/string methods and appends no offset, so to_flatbuffer<MyConfig>(std::vector<T>{...}) can succeed with the elements silently dropped. The FlatBuffers sequence layout decision needs to use the same Config-aware wire-type detection before dispatching here.
Useful? React with 👍 / 👎.
| using entry_t = std::ranges::range_value_t<Container>; | ||
| using key_t = kota::map_entry_key_t<entry_t>; |
There was a problem hiding this comment.
When a map key type is encoded through a value-mode serialize_visit to a different sortable wire type, this uses the raw container key type for ordering and may leave entries unsorted if the raw key itself is not orderable. map_view later reads keys as deep_clean_t<K> (the wire type) and binary-searches by that value, so an unsorted map such as ticket keys written as uint32_t can make valid lookups miss; collect/sort the same wire keys that are written to the entry table.
Useful? React with 👍 / 👎.
| } else if constexpr(meta::reflectable_class<T0> && !proxy_detail::is_map_range_v<T0> && | ||
| !proxy_detail::is_range_like_v<T0> && !meta::str_like<T0>) { | ||
| ok = verify_struct_slots<T0, Config>(v, root); |
There was a problem hiding this comment.
When the top-level type is reflectable but also has a value-mode serialize_visit with a wire_type, encode_value/decode_value take the adapter before treating it as a structure, but this root verifier enters the reflected-struct branch first. A valid root encoded as the adapter's scalar/string/table wire type is then walked as the raw aggregate fields and rejected with buffer verification failed before from_flatbuffer can decode it; root verification should honor the same wire-type dispatch as verify_field.
Useful? React with 👍 / 👎.
1 participant
Summary
Fixes a batch of correctness and robustness issues in the reflection-driven FlatBuffers codec, unblocking direct automatic (de)serialization of clice's index types (see the companion clice PR).
Map protocol
.first/.second(llvm::detail::DenseMapPair), exposegetKey()/getValue()(llvm::StringMapEntry), or be plain two-field aggregates. Encode, decode and runtimetype_infoall share the same accessors; decode normalizes view-typed keys (string_view,llvm::StringRef) into owning storage before insertion.map_view::find's numeric binary search ("10" < "9"), and struct keys did not compile at all. Entries now sort by the key's own ordering, which is exactly whatmap_viewcompares against.Encoding correctness
two_passreturnsnulloptinstead of a zero offset thatvisit_structsilently swallowed; variant payload write failures are no longer ignored.long doubleis encoded asdoubleeverywhere; the decoder always read 8 bytes while the encoder wrotesizeof(long double), corrupting round-trips.std::is_trivialto trivially-copyable + standard-layout + default-constructible: inline structs are memcpy'd, so default member initializers are fine and no longer demote common range-like structs to full tables.T()instead ofT{}: aggregate copy-list-initialization rejects members whose default constructor is explicit (e.g.llvm::DenseMap).Verified decoding
verify_flatbuffer<T>(bytes): schema-driven deep verification that bounds-checks every table, vector, string and scalar reachable throughT's reflected layout.from_flatbufferruns it before decoding, so corrupt or truncated buffers become a clean error instead of out-of-bounds reads. It is also exposed standalone for zero-copytable_viewreaders that never run the decoder. Opaque adapters (nowire_type) are skipped, documented in the header.Tests
flatbuffers_map_protocol_tests.cpp: scrambled integer / u64 / inline-struct keys withmap_viewlookup + wire-order assertions,StringMap-style keyed-entry container, two-field-aggregate entry container,std::mapregression round-trip.flatbuffers_robustness_tests.cpp: kitchen-sink verify + round-trip, wrong-root-type decode, per-length truncation sweep, 3-pattern byte-flip fuzz (clean under ASan/UBSan), nested encode-failure propagation,long doubleround-trip.🤖 Generated with Claude Code
Summary by CodeRabbit
verify_flatbufferoverloads for byte spans.