From 9803d3b7687c7e6569cec977bfd1b1add49c1ef4 Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Mon, 7 Sep 2026 10:58:49 +0000 Subject: [PATCH] Pre-publish review #008: gate int_keys on canonical keys, restore no-prealloc array rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review #008 (docs/reviews/008-pre-publish-review.md) audits the two post-#007 unreviewed commits (dea96f0 bench port, d4635d2 perf) before the first crates.io publish of 0.3.0. - F1: the int_keys integer dispatch accepted non-canonical mapping keys ("01", "+1" parse as u64 1) — the reader dispatched disc 1 while the materializer, validation plan, and tunion rejected the same buffer. compile_int_keys now builds the table only when every key is canonical (v.to_string() == key); otherwise the string fallback applies (agreement restored, perf kept for canonical mappings). Tests: r8_non_canonical_mapping_key_disables_int_dispatch, r8_canonical_mapping_keys_keep_int_dispatch. - F2: d4635d2 reintroduced Vec::with_capacity(count) on both array materializers. Bounded per array by MAX_ARRAY_ELEMENTS but nesting compounds: probe (counting allocator) measured ~477 MB simultaneous allocation from a ~1 KB schema + empty buffer (100-level stride-0 chain, all legal under the caps). H1's layer-1 rule restored: Vec::new() + push. Bench unchanged (packet read 220µs vs 246µs baseline). Test: r8_deeply_nested_stride0_array_rejects_before_bulk_prealloc. - N3a disposition (review #007's deferred item): BastField::synthetic (pub(crate), zero callers, #[allow(dead_code)]) deleted; the seven source() accessors are public API and stay (semver decision — removal needs an explicit ask); resolve_typeref_as_def's inline struct/union/enum arms probe-verified reachable (inline struct union variants are legal) — kept. - CHANGELOG: 0.3.0 entry (compiled forms, breaking surface, hardening fixes, coverage). README: ReadPlan/ValidationPlan roles, union conventions, untrusted-schema bounds. Verification: 569 tests green (491 lib + 17 + 34 + 15 + 12, + 2 ignored doctests), clippy -D warnings clean, cargo doc 0 warnings, wasm32 build green, cargo publish --dry-run clean. --- CHANGELOG.md | 108 +++++++++ README.md | 29 ++- docs/reviews/008-pre-publish-review.md | 296 +++++++++++++++++++++++++ src/bast.rs | 18 -- src/materialize.rs | 36 ++- src/read_plan.rs | 62 +++++- 6 files changed, 520 insertions(+), 29 deletions(-) create mode 100644 docs/reviews/008-pre-publish-review.md diff --git a/CHANGELOG.md b/CHANGELOG.md index e74288c..f5411ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,113 @@ All notable changes to this crate are documented here. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this crate adheres to [Semantic Versioning](https://semver.org/). +## [0.3.0] - 2026-09-07 + +The compiled-forms release. The packed read path — the hot path for +stream parsing — is driven by a compile-once `ReadPlan` instead of a +per-call walk of the BAST typed tree; byte validation runs a compiled +`ValidationPlan`; plans and offset maps fingerprint to a stable hash +(ADR-011/ADR-012). Reads of SFTP-shaped packet streams went from +~189× hand-rolled Rust to ~74× (~2.4× faster), fixed-stride chunk +reads from ~18× to ~11×, and `SequentialReader::read_next_borrowed` +makes the per-field hot loop allocation-free. + +### Breaking changes + +- **`Bast*` types are owned.** `BastDoc`/`BastStruct`/`BastField`/… no + longer borrow from the source `serde_json::Value`; all v0.2.0 + lifetimes are gone. `BastDoc::new` parses the root eagerly; `$ref`s + resolve lazily. +- **`OffsetMap::get` / `PackedLayout::get`** return `&OffsetEntry` + (was `Option` by value), backed by an O(log n) + `BTreeMap` path→index (first-occurrence-wins for duplicate names). +- **`SequentialReader::new`** takes the compiled plan; construct via + `AlkTypeEngine::sequential_reader()` (packed mode only). +- **`materialize_packed` / `materialize_aligned`** take the compiled + plan / `(&BastDoc, &OffsetMap)` pair respectively. +- **Field-name-discriminator union wire convention** (ADR-011 + addendum): the builder lays out the union's declared `fields` + (shared) first, then the variant's own fields. Variants must not + re-declare the discriminator or any shared field, and the + discriminator field must be the first entry in `fields` — all + enforced at parse with clean `Schema` errors. Schemas relying on + 0.2.0's variant-only layout are rejected (they produced + reader↔builder-disagreeing bytes). +- **`maxLength` is string/bytes-only** — rejected at parse on every + other kind (it was silently unenforced there). +- **Aligned-mode `Record` fields reject `offset-indirect`** (the + materializer always walks the inline count-prefixed form — the + annotated shape was never readable). +- **Schema input bounds** (untrusted-schema hardening, AGENTS.md §3): + array `count` ≤ 2^16 and `count × stride` ≤ 2^26 bytes; `align` ≤ + 4096; `maxLength` ≤ 2^26; cyclic `$ref` graphs and >128-deep nesting + are rejected by every public walker (`OffsetMap::compute`, + `LayoutBuilder::new`, `materialize_aligned` included), not just the + engine. + +### Additions + +- **`ReadPlan`** (ADR-011) — the compiled packed-read plan, re-exported + with `CompositePlan`/`FieldPlan`/`ReadKind`/`DiscriminatorPlan`. + `ReadPlan::compile` is untrusted-input-safe standalone (depth cap + + cycle set). `fixed_size()` exposes the compile-time-known byte size + for fixed structs. +- **`ValidationPlan`** (ADR-012 §3) — the compiled `validate_bytes` + walker, with `ValidNode`/`ValidVariant` sub-types. +- **`fingerprint()`** on `ReadPlan`/`OffsetMap`/`ValidationPlan` + + `Hash`/`Eq` derives on the plan types (ADR-012 §1/§4) — plan + identity for cache-keying across processes. +- **`OffsetMap` `LeafMeta`** — each entry records whether it is + fixed/length-prefixed/offset-indirect so `read_field`/`write_field` + dispatch without re-walking the schema; `OffsetEntry` type re-exported. +- **`SequentialReader::read_next_borrowed`** — zero-allocation variant + of `read_next` (field name borrowed from the plan). +- **`AlkTypeEngine::validate_bytes`** now runs the compiled + `ValidationPlan` (was an interpretive BAST walk in 0.2.0). + +### Fixes (post-release-commit hardening — reviews #006, #007, #008) + +All found and fixed before the first crates.io publish of 0.3.0, so +no published version ever exhibited them. + +- **Untrusted-input crashes removed.** A huge declared array count + OOM-aborted the process (`Vec::with_capacity(count)` before reading + a byte) — now compile-capped and walked with push-only growth. + Cyclic `$ref` graphs stack-overflowed the three standalone layout + walkers — now guarded by a shared reference-graph check. Deeply + nested stride-0 arrays briefly allowed ~477 MB of simultaneous + allocation from a ~1 KB schema — restored to incremental growth. +- **Cross-consumer divergences closed.** Builder, reader, + materializer, tunion, and the validation plan now agree on + field-disc union layout (shared-then-variant), on the discriminator + field's position (must be first), and on union mapping-key matching + (numeric fast-path dispatch only for canonical keys like `"2"`; + `"01"`/`"+1"` fall back to the string comparison all consumers + share). The legacy BAST walker's field-disc union arm walks shared + fields before the variant (it previously materialized variant fields + from shared fields' bytes). +- **Silently-corrupt layouts rejected.** Aligned record fields with + `maxLength`/`offset-indirect`; non-final inline length-prefixed + fields (records included — the ADR-006 check now sees them); + aligned-mode `maxLength`/`offset-indirect` on records; unions in + aligned mode (pre-existing, now tested). +- **Coverage**: 90.67% lines / 86.32% functions at review #007's + audit, 91.66% after its fixes; every uncovered region outside test + modules read and classified in-tree (docs/reviews/007). + +### Non-breaking improvements + +- Engine compile is one-shot and allocation-tidy; plans are + `Send + Sync` (statically asserted) and fingerprintable. +- Zero-progress array-element guard on all three array walkers (a + zero-size element makes the declared count unbounded on the wire). +- WASM-clean unchanged: two dependencies (`jsonschema` + default-features off, `serde_json` with `preserve_order`), no + `async`, no `unsafe`, no feature flags. +- Benches (`benches/wire_vs_bast.rs`): read/write chunk streams, an + SFTP-shaped union packet stream, and `validate_bytes` per buffer — + the numbers quoted above and in ADR-007/ADR-011. + ## [0.2.0] - 2026-08-17 A breaking release that replaces the v0.1.0 `AlkType:*` custom-keyword @@ -115,5 +222,6 @@ Initial crates.io release. Custom-keyword JSON Schema format `AlkTypeEngine` with packed/aligned layout modes, builder API producing `serde_json::Value`. +[0.3.0]: https://git.alk.dev/alkdev/alktype/releases/tag/v0.3.0 [0.2.0]: https://git.alk.dev/alkdev/alktype/releases/tag/v0.2.0 [0.1.0]: https://git.alk.dev/alkdev/alktype/releases/tag/v0.1.0 \ No newline at end of file diff --git a/README.md b/README.md index 506bb86..b182308 100644 --- a/README.md +++ b/README.md @@ -24,10 +24,11 @@ A BAST document serves three roles simultaneously: | Role | Mechanism | When | |------|-----------|------| -| **Validation spec (bytes)** | BAST-native validator (recursive walker over the BAST type tree) | Access time (`validate_bytes`) | +| **Validation spec (bytes)** | Compiled `ValidationPlan` walk over the materialized `Value` (ADR-012) | Access time (`validate_bytes`) | | **Validation spec (JSON)** | Standard `jsonschema::Validator` from a consumer-provided JSON Schema | Load time (build validator), access time (`validate_json`) | | **Layout spec** | Offset computation from type sizes + field order | Load time (build offset map / packed layout) | | **Data access** | Read/write at computed offsets | Access time (read field, write field) | +| **Wire access (packed)** | Compiled `ReadPlan` (ADR-011) — compile-once, no per-read schema walk | Access time (`SequentialReader`) | No separate format definition, no separate parser, no separate validator. The BAST document is the single source of truth for the @@ -159,11 +160,16 @@ same BAST document can be compiled in either mode. Decided in ADR-002. - **Byte-offset** — a fixed-size integer (`uint8`/`uint16`/`uint32`) at a known byte offset. The SFTP `Packet` pattern: byte 0 is the type byte, bytes 1..N are the variant struct. Mapping keys are stringified - integers. + integers. With all-canonical numeric keys the compiled reader + dispatches on the raw integer (no per-read stringification). - **Field-name** — a named field within the union. The TypeBox `typedef.ts` pattern. Mapping keys are string values matching the discriminator field's value. The `fields` array declares the - discriminator field (D-BAST-005). + discriminator field (D-BAST-005), which must be its first entry; the + variant must not re-declare it or any shared field. The builder lays + out the declared `fields` first, then the variant's own fields + (ADR-011 addendum) — builder, reader, materializer, and validator all + agree on that convention. Variant `$ref`s are resolved lazily — no compile-time inlining step. @@ -182,10 +188,10 @@ types (ADR-VAL-SPLIT): - `validate_bytes(&[u8])` — for raw byte buffers (channels' chunk header, SFTP packets). Materializes a `Value` tree from the bytes via - the layout engine, then runs the **BAST-native validator** — a - recursive walker over the BAST type tree that checks the value-domain - constraints the materializer doesn't (integer ranges, `maxLength`, - enum index bounds, union variant constraints). No + the layout engine, then runs the compiled **`ValidationPlan`** (0.2.0 + used an interpretive BAST walker; 0.3.0 compiles the value-domain + constraints — integer ranges, `maxLength`, enum index bounds, union + variant dispatch — once at compile time). No `jsonschema` involvement; the BAST document is the complete validation spec for bytes (D-BAST-006). - `validate_json(&Value)` / `is_valid_json(&Value)` — for already-parsed @@ -244,6 +250,15 @@ code was converted to `Err` ahead of v0.1.0 (review #002, L2); the BAST parser preserves this invariant — overflow-safe arithmetic (`checked_add`, `usize::try_from`) on all offset/count casts. +0.3.0 adds compile-time bounds for adversarial schemas: array counts +≤ 2^16 elements, computed array sizes ≤ 2^26 bytes, `align` ≤ 4096, +`maxLength` ≤ 2^26, and a shared reference-graph guard that rejects +cyclic `$ref`s and >128-deep nesting in every public schema walker. +Adversarial buffers fail with `Access` errors at read time — the +materializers never preallocate from declared counts. Reviews #006, +#007, and #008 document the audit trail +([docs/reviews/](docs/reviews/)). + ## Documentation Architecture documentation lives under [`docs/architecture/`](docs/architecture/): diff --git a/docs/reviews/008-pre-publish-review.md b/docs/reviews/008-pre-publish-review.md new file mode 100644 index 0000000..1d8fe71 --- /dev/null +++ b/docs/reviews/008-pre-publish-review.md @@ -0,0 +1,296 @@ +--- +status: resolved (F1, F2 fixed 2026-09-07; N1, N2, N3 classified; N4 fixed 2026-09-07) +last_updated: 2026-09-07 +reviewed_artifacts: + - src/read_plan.rs + - src/sequential_reader.rs + - src/materialize.rs + - src/bast.rs + - benches/wire_vs_bast.rs + - docs/reviews/007-coverage-audit.md (N3a disposition) +tool: manual diff review of post-#007 commits (dea96f0, d4635d2) + disposable probe tests (run in-session, then deleted) + cargo bench + counting-allocator peak-RSS probe +reviewer: pre-publish review #008 (session request — audit the two post-#007 perf/bench commits, then gate the 0.3.0 publish) +--- + +# Review #008 — Pre-Publish Review: Post-#007 Perf Commits + +## Purpose + +0.3.0's release commit (`9949f91`) predates review #006 entirely; the +fix sessions for #006 and #007 landed eleven more commits, and *after* +#007 closed, two more commits landed unreviewed: `dea96f0` (bench port +from alktty) and `d4635d2` (the perf commit — fixed-size struct fast +path, integer union dispatch, `read_next_borrowed`). The perf commit +touches the flagship packed read path, which every untrusted wire +buffer flows through. This review audits those two commits before the +first crates.io publish of the 0.3.x line (0.1.0 and 0.2.0 are +published; 0.3.0 never was — every post-release fix can legally ride +inside the first published 0.3.0, no semver conflict). + +It also disposes of review #007's N3a — the one finding explicitly +deferred to "the pre-release review", which this session is. + +## Methodology + +- Full diff read of `d4635d2` (perf) and `dea96f0` (bench port), + cross-checked against the invariants the earlier reviews established: + cross-consumer dispatch agreement (#006 H3, #007 F1), the H1 + no-count-sized-prealloc rule, and the N2/F2 dual-layer cap pattern. +- Disposable probe tests (`tests/zzz_probe*.rs`, deleted after the + session; none was a crash hazard) to confirm/deny the three + behaviors code reading flagged: the `int_keys` non-canonical-key + divergence, the engine's nested-array acceptance envelope, and the + `with_capacity` amplification. +- A counting-`GlobalAlloc` probe (peak-bytes metric) to measure the + worst-case simultaneous allocation of the amplification shape + precisely — RSS timing proved too noisy to separate the two test + cases. +- `cargo bench --quick` before/after the fixes to confirm the perf + commit's wins survive. +- N3a dispositions probed where cheap (inline-struct union variants). + +## Baseline + +Audited at `main` HEAD `d4635d2`, 0.3.0, working tree clean. 566 +tests green (488 lib + 17 + 34 + 15 + 12, + 2 ignored doctests), +clippy `-D warnings` clean, wasm build green — per the perf commit's +verification block. + +## Summary Statistics + +| Severity | Count | Status | +|----------|------:|--------| +| High | 0 | — | +| Medium | 2 (F1, F2) | both fixed 2026-09-07 | +| Info | 3 (N1, N2, N3) | classified | +| Fix | 1 (N4) | fixed 2026-09-07 | + +No Highs: both Mediums are probe-verified cross-consumer divergences +and resource-bound violations, but neither aborts the process +(`with_capacity` is now bounded per array by `MAX_ARRAY_ELEMENTS`, so +H1's 1 TB SIGABRT class does not return). Both were fixed in-session +because they violate AGENTS.md §3 (divergent verdicts on untrusted +input; unbounded-count-shaped allocation) — the publish gate. + +**Resolution log:** + +- **F1 + F2 (2026-09-07):** fixed in one commit — see the resolution + blocks. 569 tests green (491 lib + 17 + 34 + 15 + 12, + 2 ignored), + clippy `-D warnings` clean, doc 0 warnings, wasm green. +- **N4 (2026-09-07):** fixed with F1/F2 — see the block. + +--- + +## Findings + +### F1. `int_keys` integer dispatch breaks cross-consumer agreement on non-canonical mapping keys + +**Files**: `src/read_plan.rs` (`compile_int_keys`, introduced by +`d4635d2`), contrast `src/materialize.rs` (`materialize_plan_union`'s +byte-disc arm — stringifies), `src/validation_plan.rs` +(`validate_union_numeric` — stringifies), `src/tunion.rs` +(`read_byte_discriminator` — stringifies) + +**Problem**: `d4635d2` added a pre-parsed `(u64, variant_index)` +dispatch table for byte-discriminator unions: when every mapping key +parses as `u64`, the reader matches the raw discriminator integer +instead of stringifying per read. But the meta-schema does not +constrain mapping-key shape beyond "object property name", and +`key.parse::()` accepts **non-canonical** decimal strings: + +``` +PROBE1 reader: field=msg disc="01" (DISPATCHED) +PROBE1 validate_bytes: Err(access error at msg: union discriminator value 1 not in mapping) +``` + +With mapping key `"01"` (and discriminator `1` on the wire): the +reader's numeric dispatch **matches** (`"01".parse::() == 1`) and +dispatches — returning `discriminator == "01"` — while the +materializer (`1.to_string() == "1" ≠ "01"`), the validation plan, and +tunion all **reject** the identical buffer. Pre-`d4635d2`, all four +consumers stringified and all four rejected — agreement held (both +verdicts "reject", same error class). The perf commit flipped the +reader to accept-while-everyone-else-rejects: the exact +cross-consumer-divergence shape #006 H3 and #007 F1 exist for, on the +flagship path. `"+1"` parses as `u64` too (Rust's `from_str_radix` +accepts a leading `+`) — same class. The returned key string also +became schema-quirk-dependent: the reader reports `"01"` where the +materializer's `__discriminator` for a *matched* key would report the +stringified form. + +**Not a #007 regression**: `int_keys` did not exist before `d4635d2`. +But `d4635d2` postdates #007's close and was unreviewed — this is the +audit catching it. + +**Fix**: build the integer table only from **canonical** keys — a key +qualifies iff `key.parse::()` succeeds *and* +`parsed.to_string() == key` (i.e. the key is exactly what +stringification would produce). Any non-canonical or non-numeric key +falls back to the string path (`int_keys: None`), which every consumer +already agrees on. No accepted schema's *reachable* behavior changed: +for fully-canonical mappings the numeric dispatch behaves identically +to stringified matching (the numeric value's `to_string()` equals the +key), and for non-canonical keys all consumers now reject exactly as +before `d4635d2`. The perf win (no per-read stringify) is preserved +for every mapping that was unambiguous to begin with. + +**Resolution (2026-09-07):** exactly that — `compile_int_keys` now +requires `v.to_string() == *key` for the table to carry the entry; +any miss returns `Ok(None)` (string fallback). Doc comment states the +canonicality rule and why. Tests in `read_plan.rs`: +`r8_non_canonical_mapping_key_disables_int_dispatch` (key `"01"` → +`int_keys` is `None`) and `r8_canonical_mapping_keys_keep_int_dispatch` +(keys `"1"`,`"2"` → table `[(1,0),(2,1)]`). Probe output after the fix: +both `validate_bytes` and the reader reject disc 1 under key `"01"` +with the same error class — agreement restored. + +### F2. `Vec::with_capacity(count)` reintroduced on both array materializers — ~477 MB simultaneous allocation from a ~1 KB schema + +**Files**: `src/materialize.rs:247` (`materialize_plan_array` — the +`validate_bytes` packed path), `src/materialize.rs:650` +(`materialize_array_packed` — the legacy walker, reachable via the +aligned record arm) + +**Problem**: `d4635d2`'s "materialize: with_capacity for bytes arrays, +arrays, and struct objects" item reintroduced +`Vec::with_capacity(count)` at two of the three sites H1's layer-1 fix +had converted to `Vec::new()` + push. `count` is now compile-capped at +`MAX_ARRAY_ELEMENTS` (2^16), so H1's 1 TB `SIGABRT` does not return — +but the per-array cap does not bound *nesting*: + +``` +PROBE validate peak bytes allocated simultaneously: 476780249 +``` + +A schema of one 100-level nested array chain (each `count: 65535`, +innermost elements empty structs — all legal: the depth cap is 128 and +stride-0 chains evade `MAX_ARRAY_BYTES`, which only checks stride +products) peaks at **~477 MB of simultaneous allocation** on +`validate_bytes(&[])` from a ~1 KB schema and an *empty* buffer. Each +level's `with_capacity(65535 × sizeof(Value))` stays live across its +element walk, so the sizes multiply across ~127 legal depth levels +(the innermost zero-progress rejection fires only after the whole +chain has descended). On wasm32 — which this crate explicitly targets +— the same shape aborts the wasm heap well below 477 MB. H1's layer-1 +rule ("no count-sized prealloc on untrusted input; the per-element +walk dominates") is exactly the invariant this violates; the perf +commit's own bench evidence doesn't need the prealloc either (see +below). + +The other `with_capacity` additions in the commit are fine: byte-array +capacity from `b.len()` (a read slice), struct-object capacity from +`plan.fields().len()`, and the plan-compiler's from `fields.len()` — +all bounded by data/plan already in hand, not by declared counts. + +**Fix**: restore H1's layer-1 shape at both sites — `Vec::new()` + +push loop (the loops already push `count` elements; the zero-progress +guard bounds honest progress per element). Optionally cap +preallocation at a small constant, but plain `Vec::new()` matches H1's +shipped behavior. + +**Resolution (2026-09-07):** both sites restored to `Vec::new()` + +push. Bench before/after (criterion `--quick`, this session): packet +read 246 → 220 µs, chunk read 76/68 µs — the revert costs nothing +measurable on the bench shapes (small arrays; the materializer's +per-element work dominates), and the union/struct preallocs stay. +Locking test in `materialize.rs`: +`r8_deeply_nested_stride0_array_rejects_before_bulk_prealloc` (the +100-level chain still rejects cleanly with the zero-progress error at +the innermost level; the allocation shape itself is documented here — +in-tree cannot cheaply assert peak allocation, and the #008 probe was +deleted per the no-reproducer rule). + +### N1. `d4635d2`'s fixed-size fast paths are sound (classified, no action) + +**Files**: `src/read_plan.rs` (`fixed_size`, `fixed_plan_size`), `src/sequential_reader.rs` + +The fixed-size struct fast path replaces the cursor size walk with one +bounds check; `fixed_plan_size` already returned `Result