Review #006: record L4/N1/M4 closure; add M5/M6 (fixed) and N3 (open)

- Resolution blocks on L4 (BTreeMap index + first-occurrence-wins
  pinned), N1 (tunion extended), M4 items 1-4 (aligned family, reader
  arms, data-access guards, dead-arm verdict with the structural note
  that the legacy packed walker serves only aligned fallbacks now).
- New findings: M5 (aligned record maxLength/offset-indirect silent
  corruption, resolved same-day), M6 (legacy walker field-disc union
  skipped shared fields, resolved same-day), N3 (packed record
  maxLength unenforced in validate_bytes, open - posture decision).
- Stats, resolution log, recommended order, and notes updated.
This commit is contained in:
glm-5.3-flash committed 2026-09-02 20:28:05 +00:00
1 parent 2eb086f400
commit 0857ea1c23
1 file changed
+326 -18
+326 -18
View File
@@ -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<String, usize>` 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<uint16> 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<union-field-disc>: {"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<union-field-disc>: {"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<uint16> 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.
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.