diff --git a/docs/reviews/006-implementation-review-030.md b/docs/reviews/006-implementation-review-030.md index 751b2a5..f4de027 100644 --- a/docs/reviews/006-implementation-review-030.md +++ b/docs/reviews/006-implementation-review-030.md @@ -1,5 +1,5 @@ --- -status: in-progress (H1, L1, H3, L5, L6, H2, M1, M2, M3, L2, L3, N2 resolved 2026-09-02) +status: in-progress (H1, L1, H3, L5, L6, H2, M1, M2, M3, L2, L3, N2, L4, N1, M5, M6 resolved 2026-09-02) last_updated: 2026-09-02 reviewed_artifacts: - src/read_plan.rs @@ -96,13 +96,16 @@ sub-types, `fingerprint()` methods; `Hash` on `Endian`/ | Severity | Count | Status | |----------|------:|--------| | High | 3 (H1, H2, H3) | all resolved 2026-09-02 | -| Medium | 4 (M1, M2, M3, M4) | M1, M2, M3 resolved 2026-09-02; M4 ongoing | -| Low | 6 (L1–L6) | L1, L2, L3, L5, L6 resolved 2026-09-02 | -| Nit | 2 (N1, N2) | N2 resolved 2026-09-02; N1 open | +| Medium | 6 (M1–M4, M5, M6) | M1, M2, M3, M5, M6 resolved 2026-09-02; M4 items 1–4 closed 2026-09-02 (posture ongoing) | +| Low | 6 (L1–L6) | all resolved 2026-09-02 | +| Nit | 3 (N1, N2, N3) | N1, N2 resolved 2026-09-02; N3 open | -All three Highs, three of four Mediums, five of six Lows, and one of -two Nits are resolved. Remaining: M4 (ongoing per-fix coverage -posture), L4, N1. +All Highs, five of six original Mediums (M4's four concrete items are +closed; the per-fix coverage posture itself is ongoing by design), all +six Lows, and two of three Nits are resolved. Two new findings (M5, M6) +and one new Nit (N3) surfaced during the follow-up session while +closing M4's coverage map — all in the same untrusted-input family the +review exists for. Remaining: M4 (ongoing posture), N3. The three Highs are adversarial-input crashes (H1, H2) and a cross-consumer wire-layout convention gap (H3) — all three are the @@ -149,6 +152,18 @@ them became materially easier to hit with the 0.3.0 surface. retuned to the new legal maximum. 511 tests green, clippy `-D warnings` clean, wasm build green, `cargo doc --no-deps` zero warnings. +- **L4 + N1 + M4 item 4 + M5 (2026-09-02):** resolved in one commit — + see the resolution blocks on each finding. M4 item 4's investigation + verdict (dead arms, not a divergence) is recorded there; M5 was a + probe-verified new finding in the M1 family. 546 tests green, clippy + `-D warnings` clean, wasm build green. +- **M6 + M4 items 1–3 (2026-09-02):** resolved in one commit — see the + resolution blocks. M6 was probe-verified while writing the M4 + record-with-union-values coverage test; items 1–3 closed the aligned + materializer, reader, and data-access holes from M4's map. Coverage + after: materialize.rs 64.48→85.72% lines, TOTAL 89.59→90.60%. 567 + tests green, clippy `-D warnings` clean, wasm build green, `cargo + doc --no-deps` zero warnings. --- @@ -758,6 +773,76 @@ check into each fix session — after fixing a finding, extend adjacent uncovered branches while the context is fresh, rather than scheduling a standalone coverage sweep. +**Resolution (2026-09-02) — all four items closed in the two follow-up +commits; the per-fix posture itself stays ongoing by design:** + +1. **Aligned materializer (the big chunk)** — a 16-test family added + (`materialize.rs` `m4_aligned_*`): three-level nested-struct + recursion through `OffsetMap` + `materialize_aligned` (the 0-execution + recursion — pinned with wire arithmetic: `header.magic@0..4`, + `header.meta.ver@4..6`, `header.meta.flag@6..7`, `body@8..12`), + maxLength trim inside nested structs, the invalid-UTF-8 `Access` + error arm, offset-indirect out-of-bounds and data-after-sibling + roundtrip, and records with struct / array / byte-disc-union / + field-disc-union / wide-primitive values driving the legacy BAST + walker's previously-dead arms (`materialize_struct_packed`, + `materialize_array_packed`, `materialize_union_packed`, and the + i8..bool/float/string/bytes arms of `materialize_typeref_packed`). + The record-with-field-disc-union test probe-verified M6 (see the M6 + finding — writing this test was how M6 surfaced). Coverage: + `materialize.rs` 64.48%→85.72% lines. + + One structural insight the mapping now records: the "packed" BAST + walker family (`materialize_struct_packed` etc.) is no longer the + public packed path — `materialize_packed` drives the compiled plan + since 0.3.0. The legacy walker is reachable only through the aligned + record arm (`materialize_struct_aligned`'s Record dispatch) and + `materialize_leaf_at`. Its uncovered regions were aligned-reachable, + not dead — which is exactly why the M4 map had to be closed with + aligned-path tests. + + Two asymmetries documented while writing the tests: the aligned + walkers reject struct-element arrays (kind `Struct` fails + `is_fixed_size()` — OQ-001's conservative gate), while the packed + plan compiles fixed-struct-element arrays with a computed stride; + and the legacy walker's record walk is *packed* internally (no + padding inside record entries) even when reached from aligned mode + — the count-prefixed walk reads `key/tag/val` back-to-back. + +2. **`sequential_reader.rs`** — all four hole groups closed: public + `schema()`/`plan()` accessors called and asserted (compiled doc + + `FieldPlan` list); field-disc uint16/uint32/enum dispatch arms + driven end-to-end (wide values `"514"`/`"258"` dispatch to their + mapping keys); byte-disc uint16/uint32 arms; and + `plan_walk_variant_size`'s nested-union arm (byte-disc union whose + variant is itself a union — the capability phase 1 restored, now + read-tested). + +3. **`data_access.rs`** — indirect-write tests added at a nonzero pair + offset and for the data-region bounds refusal (the pair may write + before the data copy is refused — that partial-write behavior is + pinned). The remaining uncovered lines are the `u32`-truncation + guards and `checked_add` overflow arms, which need >4 GiB slices or + near-`usize::MAX` offsets — defensively unreachable on 64-bit test + hardware without multi-GiB allocations; documented here rather than + forced in-tree (same reasoning as H1's no-reproducer rule). + +4. **`offset_map.rs` `field_endian_for_element`** — **verdict: dead, + not a divergence.** The function's only call site + (`compute_array_field`) runs the OQ-001 rejection (`!elem_kind. + is_fixed_size()` → `Offset` error) *before* the endian lookup, and + `Struct`/`Union`/`Array`/`Record` are all non-fixed-size kinds + (`schema.rs::is_fixed_size`) — so the `Struct(s) => s.endian()` / + `Union(u) => u.endian()` arms could never execute. The apparent + contradiction with the phase-5 parity rule never existed on any + reachable path. The dead composite arms were deleted + (`element_alignment`'s composite arms were dead the same way and + were removed with it), the reachable propagation is locked by + `m4_array_element_endian_inherits_referring_field_not_element_own` + (field-level override and struct default both flow to elements), and + the OQ-001 gate for struct elements is locked by + `m4_array_of_struct_element_rejected_oq001`. + ### L1. `unwrap_or_default()` conflates "variable-length" with "checked-mul overflow" for array strides **File**: `src/read_plan.rs:547` @@ -843,6 +928,8 @@ behavioral change; 508 tests green. **Files**: `src/offset_map.rs:154-159`, `src/layout_builder.rs:83-88` +**Status**: resolved 2026-09-02. + **Problem**: both `get` implementations are `self.fields.iter().find(…)` over a `Vec` — O(n) per lookup, on the types whose docs pitch random access ("the consumer can read field N @@ -857,6 +944,23 @@ over the `Vec`) makes the claim true with no public-surface change. Not a 0.3.0 blocker; a good 0.3.x cleanup with an existing in-repo pattern to copy. +**Resolution (2026-09-02):** the recommended index, exactly. Both +types gain a private `BTreeMap` built once at +construction (`OffsetMap::compute` / `LayoutBuilder::build`); `get` +is an O(log n) map lookup + O(1) vec index. `iter()` order, the +`PartialEq`/`Eq`/`Hash` derives (over the insertion-ordered `Vec`), +and the public surface are unchanged. + +One semantic pinned before it could regress: `BastStruct::parse` does +**not** reject duplicate field names, so two same-named siblings +produce two entries with the same path. The old `find` returned the +*first*; the index uses `entry().or_insert(i)` to preserve +first-occurrence-wins. Locked by `l4_duplicate_field_paths_ +first_occurrence_wins` in both modules, plus +`l4_get_is_indexed_random_access_over_large_nested_schema` (40 nested +blocks, late dotted-path lookups, prefix-collision non-matches +`"blk"`/`"blk20"` vs `"blk20.tag"`). + ### L5. `FieldValue::Union`'s `variant_start` semantics are undocumented and inconsistent between discriminator kinds **File**: `src/sequential_reader.rs:84-91` @@ -925,6 +1029,8 @@ is the test that would have caught every part of H3. `src/sequential_reader.rs:613-645` (supports `String`/ `Uint8`/`Uint16`/`Uint32`/`Enum`) +**Status**: resolved 2026-09-02 (extend posture). + **Problem**: `tunion::read_field_discriminator` rejects uint16/uint32 discriminator fields with a `Schema` error, while the plan reader handles them. Parity-preserved (tunion is unchanged from 0.2.0), but @@ -934,6 +1040,20 @@ or document the divergence; the meta-schema does not constrain the discriminator field's kind, so both code paths are reachable from the same schema. +**Resolution (2026-09-02):** extension taken (over documentation) — +`tunion::read_field_discriminator` now handles `uint16` (LE/BE) and +`uint32` (LE/BE) disc fields with the same stringification and +`discriminator_field_size` semantics the reader's +`plan_discriminator_string_value` uses, so both public dispatch paths +answer "string/uint8/uint16/uint32/enum". The doc comment's kind list +is updated to name the parity with the reader explicitly. Four tests +(`n1_read_field_discriminator_uint16_little_endian` / +`_big_endian` / `n1_read_field_discriminator_uint32_little_endian` / +`_big_endian`) pin the new arms including endianness and +`variant_offset` arithmetic. No accepted schema's behavior changed +(schemas using uint16/uint32 disc fields previously failed tunion +dispatch with a `Schema` error; they now dispatch). + ### N2. `align` annotations are unbounded — no crash, but absurd layouts and the reachable path to H1's byte cap **Files**: `src/bast_meta.rs:64,79` (`"align": { "type": "integer", @@ -989,6 +1109,175 @@ align + maximum), `n2_field_align_above_cap_rejected_at_parse`, 4096). Verified: 511 tests green, clippy `-D warnings` clean, wasm build green, `cargo doc --no-deps` zero warnings. +### M5. Aligned `Record` fields accept `maxLength`/`offset-indirect` annotations the materializer cannot honor — silent cross-field corruption + +**Files**: `src/offset_map.rs` (`compute_field`'s `Record` arm → +`compute_variable_field`), `src/materialize.rs:891-904` (the aligned +record dispatch), `src/bast.rs:412-442` (`BastField::parse` accepts +the annotations on any field) + +**Found**: 2026-09-02, while closing M4 — a probe run to characterize +the aligned-record surface M2's tests had just locked. + +**Problem**: a `Record` field in aligned mode routes through +`compute_variable_field`, which honors `maxLength` (fixed-size +reservation) and `offset-indirect` (the 8-byte `{offset, length}` +pair) exactly as it does for strings/bytes. But the materializer's +aligned `Record` arm *always* walks the inline count-prefixed form +from the entry start (`materialize_typeref_packed` at +`offset = entry.range.start`) — it never consults the encoding. So: + +- With `maxLength`, the map records an N-byte reservation and + `total_size` accounts for it, but the materializer reads the record's + inline `count`-prefix form — the record data walks past the + reservation boundary and overlaps subsequent fields' bytes. +- With `offset-indirect`, the map records an 8-byte `{offset, length}` + pair while the materializer reads a count prefix from the same + offset — the wire shape the map promised was never read. + +Probe (verbatim): + +``` +PROBE map: counts=Some(ByteRange { start: 0, end: 8 }) id=Some(ByteRange { start: 8, end: 12 }) total=12 +PROBE id field reads: 10849 (bytes 8..12: [97, 42, 0, 0]) +PROBE materialize: {"counts":{"a":42},"id":10849} +PROBE validate: OK — record walk crossed the reservation and was accepted +``` + +i.e. `{ counts: record maxLength 8, id: uint32 }` accepts a +buffer where the record's count-prefixed data extends into `id`'s +bytes; `validate_bytes` passes and both fields materialize from the +same overlapping bytes (`id` reads `0x2A2A`'s little-endian spill as +10849). **Parity-preserved** (0.2.0's aligned materializer did the +same inline walk; the annotations were equally accepted), but it is +the M1 hazard in the *annotated* form: ADR-006's own error text +recommends `maxLength` or `offset-indirect` as the fix for +non-final inline variable fields — and for records, neither fix works. +That makes the schema-level guidance a trap for record fields. + +**This violates AGENTS.md §3** in the same "silently corrupt" sense +M1 did: the engine accepts a schema whose annotated layout cannot be +honored, then accepts corrupt buffers against it. + +**Fix**: reject the two annotations on aligned record fields with a +clean `Offset` error at compute time (walk-time, like ADR-006 — the +hazard is aligned-mode-specific; packed mode's inline form is +consistent, see N3). Position does not rescue `maxLength` (even as +the last field the walk is the wrong wire shape for the reservation), +so the rejection is unconditional within aligned mode. + +**Resolution (2026-09-02):** exactly the recommended fix. +`compute_field`'s `Record` arm rejects `encoding: offset-indirect` +("the materializer walks the record's inline count-prefixed form from +the entry start…") and `maxLength` (naming the M5 corruption shape) +with clean `Offset` errors; inline length-prefixed records remain +allowed per M1/M2's rules (last field only). Tests: +`m5_record_max_length_rejected_in_aligned_mode`, +`m5_record_offset_indirect_rejected_in_aligned_mode`, +`m5_record_max_length_rejected_even_as_last_field`. Breaking +constraint for 0.2.0-era schemas that annotated aligned records — +same class as H3's re-declaration rejection (previously ambiguous / +silently corrupt, now rejected with a clean error naming the +annotation). Verified with M5's commit: 546 tests green, clippy clean, +wasm green. + +### M6. The legacy BAST-walker's field-disc union arm skips non-discriminator shared fields — variant materializes from shared fields' bytes + +**Files**: `src/materialize.rs:728-765` (the `Field` arm of +`materialize_union_packed`), reachable via the aligned record arm and +`materialize_leaf_at` (not via `materialize_packed`, which uses the +plan-based materializer that was already correct) + +**Found**: 2026-09-02, while writing M4 item 1's +record-with-union-values coverage test — the test's assertions +failed in exactly the shape of H3 item 1's reader/builder +disagreement, but on the legacy walker. + +**Problem**: `materialize_union_packed`'s field-disc arm found the +discriminator field by name, materialized *only* it, and started the +variant immediately after — never walking the union's other declared +`fields`. Whenever a field-disc union had any non-disc shared field, +the variant materialized from the shared fields' bytes. Probe +(verbatim, the schema is union `fields: [type: uint8, seq: uint32]`, +variant `Read { handle: uint32 }`, as a record's value type): + +``` +PROBE aligned record: {"events":{"a":{"__discriminator":"1","type":1,"handle":5}}} +``` + +`handle: 5` — but 5 was `seq`'s wire value; `handle` (77) was never +read. The same union through the plan-based packed materializer +produces the correct `type=1, seq=5, handle=77` (H3's roundtrip test +pins that path). This is the H3 convention gap's last surviving +consumer: H3 fixed the builder (shared-then-variant), the plan reader +and plan materializer were already position-correct, but this legacy +arm predates the convention and was not in H3's consumer list because +no test drove a field-disc union through it. Reachable through +`materialize_aligned` on any aligned schema that places a field-disc +union inside a record value (or, in principle, any aligned composite +that falls back to the packed walker). + +Severity Medium rather than High because the shape requires a +field-disc union with non-disc shared fields nested inside an aligned +record/leaf fallback — and H3's parse rules made the *first-class* +union path correct everywhere else — but it is the same +silently-corrupt-data class. + +**Fix**: make the arm walk all declared `fields` in order (capturing +the disc value at its real position), start the variant after the +whole shared walk, and emit the same object shape the plan +materializer emits: `__discriminator` first, then the typed disc +value under its field name, then the remaining shared fields, then +the variant's fields flattened in. + +**Resolution (2026-09-02):** exactly that. The arm now walks every +shared field via `materialize_field_packed`, captures +`key`/`disc_value` by name as it passes, materializes the variant +after the walk, and assembles the object in the plan materializer's +key order. Probe output after the fix: + +``` +PROBE aligned record: {"events":{"a":{"__discriminator":"1","type":1,"seq":5,"handle":77}}} +``` + +Tests: `m4_aligned_record_with_field_disc_union_values_dispatches` +(the coverage test that found it, now asserting `type`, `seq`, +`handle`, `__discriminator`), plus +`m4_aligned_record_with_byte_disc_union_values_dispatches` for the +byte-disc arm (which needed no fix). No accepted schema's packed-mode +behavior changed — the fix only alters the legacy walker's output for +the corrupt-shape case. Verified with M4's commit: 567 tests green, +clippy `-D warnings` clean, wasm build green. + +### N3. `maxLength` on record fields is silently unenforced by `validate_bytes` in packed mode + +**Files**: `src/validation_plan.rs` (`ValidNode::Record` carries no +`max_len`; `maxLength` is baked into `Str`/`Bytes` leaves only), +`src/bast.rs:1026` (`parse_max_length` accepts it on any field) + +**Found**: 2026-09-02, while bounding the M5 fix (checking whether +`maxLength` meant anything for records anywhere). + +**Problem**: the meta-schema allows `maxLength` on any field +(including records), and the parser records it — but the validation +plan bakes `maxLength` only into string/bytes leaf nodes +(validation_plan.rs:389). A record's materialized object has no +byte-length check against the annotation. Probe: packed schema +`{ counts: record maxLength 8 }`, wire payload 22 bytes — +`validate_bytes` returns OK. No corruption (packed writer and reader +agree on the inline form), just an annotation that does nothing, on a +field kind where a consumer might reasonably expect the entry-region +size cap that ADR-006's fix text implies. Parity-preserved from +0.2.0. + +**Fix options**: (a) enforce `maxLength` on records as a cap on the +entry region's byte size in `validate_bytes` (matches ADR-006's +wording); (b) reject `maxLength` on record fields at parse (honest, +but rejects schemas that are otherwise fine); (c) document that +`maxLength` is string/bytes-only and enforce it in the meta-schema. +Option (c) is the cheapest honest closure; option (a) is the most +useful. Needs a posture decision like N1's — either is small. + --- ## What's Good @@ -1038,16 +1327,22 @@ Worth recording, because the findings shouldn't eclipse it: resolution blocks on the findings). 5. ~~**M3**~~ **resolved 2026-09-02** (see the resolution block on the finding). -6. **M4** — ongoing: per-fix coverage extension as recommended above; - the aligned-materializer test gap (1) is the single biggest chunk - and deserves its own session. (M1/M2/M3's fixes each extended - coverage over their touched paths — the aligned record dispatch and - the aligned record `validate_bytes` path are now publicly exercised.) -7. **L1–L6, N1, N2** — opportunistic, folded into whichever session - touches the relevant file (L6 is the exception — it belongs with - H3; N2 pairs naturally with any bast/bast_meta session; L1 done - with H1; L5/L6 done with H3; L2/L3 done with the M-fix session; - N2 done in the M-fix session's tail). +6. ~~**L4**~~ **resolved 2026-09-02** (BTreeMap index; see the + resolution block on the finding). +7. ~~**N1**~~ **resolved 2026-09-02** (tunion extended to match the + reader's kind set; see the resolution block on the finding). +8. ~~**M4** — coverage map~~ **items 1–4 closed 2026-09-02** (aligned + materializer family, reader arms + accessors, data-access guards, + `field_endian_for_element` verdict: dead arms deleted); the + per-fix coverage posture stays ongoing by design. The M4 session + surfaced **M5** and **M6** (both fixed same-day, see their + findings) and **N3** (open). +9. ~~**M5**~~ **resolved 2026-09-02** (aligned record + `maxLength`/`offset-indirect` rejected at compute). +10. ~~**M6**~~ **resolved 2026-09-02** (legacy walker's field-disc + union arm walks shared fields first). +11. **N3** — open: pick the posture (enforce record `maxLength` in + `validate_bytes`, or document string/bytes-only) and close. ## Notes @@ -1072,4 +1367,17 @@ Worth recording, because the findings shouldn't eclipse it: gap (stride-0 array + zero-byte elements = unbounded loop) and the unbounded-`align` observation (new N2) while re-deriving the cap arithmetic — both recorded on their own findings rather than - silently absorbed. \ No newline at end of file + silently absorbed. +- The 2026-09-02 follow-up session (L4/N1/M4 closure) re-used the + disposable-probe pattern from the Methodology section for three + characterizations: M5's record-overrun corruption (probe run in a + scratch crate, then deleted), the N3 packed-`maxLength` + unenforcement, and M6's legacy-walker divergence (the failing + assertion of a would-be coverage test). None of the three probes + was a crash hazard; the probes were deleted after use, per this + review's no-reproducer rule. +- The M4 coverage numbers quoted above (89.59%→90.60% TOTAL, + materialize.rs 64.48%→85.72%) were measured with + `cargo llvm-cov --release` before and after the session's two + commits (`5f9793f`, `2eb086f`); per-file numbers in the M4 finding + table are from the original review run and were not restated. \ No newline at end of file