diff --git a/docs/reviews/006-implementation-review-030.md b/docs/reviews/006-implementation-review-030.md index 24c8792..8fe2fa7 100644 --- a/docs/reviews/006-implementation-review-030.md +++ b/docs/reviews/006-implementation-review-030.md @@ -1,5 +1,5 @@ --- -status: open +status: in-progress (H1, L1 resolved 2026-09-02) last_updated: 2026-09-02 reviewed_artifacts: - src/read_plan.rs @@ -93,21 +93,31 @@ sub-types, `fingerprint()` methods; `Hash` on `Endian`/ ## Summary Statistics -| Severity | Count | -|----------|------:| -| High | 3 (H1, H2, H3) | -| Medium | 4 (M1, M2, M3, M4) | -| Low | 6 (L1–L6) | -| Nit | 1 (N1) | +| Severity | Count | Status | +|----------|------:|--------| +| High | 3 (H1, H2, H3) | H1 resolved 2026-09-02 | +| Medium | 4 (M1, M2, M3, M4) | open | +| Low | 6 (L1–L6) | L1 resolved 2026-09-02 | +| Nit | 2 (N1, N2) | open | The three Highs are adversarial-input crashes (H1, H2) and a cross-consumer wire-layout convention gap (H3) — all three are the exact class of problem AGENTS.md §3 exists for, given that `alkcall` -feeds schemas from arbitrary internet peers into this engine. H1 should -block publishing 0.3.0 to crates.io until fixed. None of the three is a -0.2.0 regression in the strict sense (details per finding), but two of +feeds schemas from arbitrary internet peers into this engine. H1 +blocked publishing 0.3.0 to crates.io until fixed (now fixed, see the +resolution note on the finding). None of the three is a 0.2.0 +regression in the strict sense (details per finding), but two of them became materially easier to hit with the 0.3.0 surface. +**Resolution log:** + +- **H1 + L1 (2026-09-02):** resolved in one commit — see the + "Resolution (2026-09-02)" block at the end of the H1 finding and the + L1 finding. 482 tests green (405 crate + 67 integration + 2 + pre-existing ignored), clippy `-D warnings` clean, wasm build green. + The fix session also surfaced a new Nit (N2, unbounded `align` + annotations) — added below. + --- ## Findings @@ -177,6 +187,65 @@ Add tests: compile+validate with `count: usize::MAX` (expect clean `Err`), and a moderate count against a short buffer (expect clean `Err`). +**Resolution (2026-09-02) — both layers, plus two gaps the original fix +text missed:** + +1. **Abort removal (fix layer 1):** all three `Vec::with_capacity(count)` + sites are now `Vec::new()` + push loop (`materialize_plan_array`, + `materialize_array_packed`, `materialize_array_aligned`). The + `validate_bytes` OOM-abort is gone: with an oversized count the + per-element walk now errors on buffer bounds instead of allocating. + +2. **Compile-time caps (fix layer 2), at two choke points** — the + review's "documented cap" suggestion landed as two constants in + `schema.rs`: + - `MAX_ARRAY_ELEMENTS = 2^16`, enforced in `BastArray::parse` + (`bast.rs`) — the single point every consumer inherits + (`BastDoc::new` is the only construction path, and all five + consumers — engine, ReadPlan, LayoutBuilder, OffsetMap, and + standalone `BastDoc` users — parse through it). This cap is what + bounds the walkers' *per-element push loops*: with a product cap + alone, 256 MiB of u8 elements would still mean 268M loop + iterations accumulating ~20 GB of offset-map/layout entries at + compile time. The original fix text ("bound `count × element + size`") missed this; a product cap bounds wire size, not walker + memory. + - `MAX_ARRAY_BYTES = 2^26`, enforced per walker against the + mode-appropriate stride: `compile_array` (ReadPlan, schema error), + `walk_array` (LayoutBuilder, offset error — defense-in-depth + there: packed mode has no alignment, so max fixed element 8 × + count 2^16 = 2^19 << 2^26 can't reach the cap), `compute_array_field` + (OffsetMap, offset error — *reachable* there, via an + align-driven stride, see below). + - L1 folded in: `fixed_composite_size`/`fixed_plan_size` now return + `Result>`; `unwrap_or_default()` is gone, and the + checked-mul overflow arm is a clean `Schema` error instead of a + silent stride-0. + +3. **Zero-progress runtime guards** — a gap neither the finding nor the + fix text covered: a stride-0 array whose elements consume 0 bytes + (empty-struct elements are legal; `stride 0` means per-element + sequential walking) loops `count` times with no buffer bound even + after both caps. `plan_walk_variable_array_size` (reader) and + `materialize_array_packed` (materializer) now error with "array + element consumed 0 bytes" when an element makes no wire progress. + The aligned materializer needs no guard: aligned array elements are + always fixed-size kinds (`type_size() >= 1`). + +Tests (8 new + 1 rewritten): parse-cap rejection (bast), byte-cap +rejection via ReadPlan/LayoutBuilder/OffsetMap, count-at-cap +acceptance, packed 2^16-array build acceptance (layout_builder, +documents why the byte cap is unreachable in packed mode), huge-count +short-buffer clean access error, and the old +`array_count_large_u64_parses_on_64bit` (which asserted the *old* +unbounded-parse behavior as a feature) rewritten as +`array_count_large_u64_rejected_by_compile_cap`. No OOM reproducers +in-tree (per the Methodology warning); the in-tree tests assert only +the compile-time rejections — the safe half. + +Verified: 482 tests green, clippy `-D warnings` clean, +`cargo build --target wasm32-unknown-unknown --release` green. + ### H2. Cyclic `$ref` stack-overflows `OffsetMap::compute` / `LayoutBuilder::new` / `materialize_aligned` when driven standalone **Files**: `src/offset_map.rs:123` (`compute`), @@ -492,6 +561,8 @@ scheduling a standalone coverage sweep. **File**: `src/read_plan.rs:547` +**Status**: resolved 2026-09-02 (with H1). + **Problem**: `let element_stride = fixed_composite_size(&element) .unwrap_or_default();` — `fixed_composite_size` returns `None` both for genuine variable-length elements (correct → stride 0) and for @@ -504,6 +575,13 @@ call site. Prefer making `compile_array` return `Err` on the overflow arm (a small refactor of `fixed_composite_size` to return a `Result, …>` or to take the cap from H1's fix). +**Resolution (2026-09-02):** `fixed_composite_size` and +`fixed_plan_size` now return `Result, AlkTypeError>`; +`None` means variable-length only, and overflow arms return clean +`Schema` errors. `compile_array` uses `fixed_composite_size(&element)? +.unwrap_or(0)` — the conflation is gone. See the H1 resolution block +for the full change description. + ### L2. `ReadPlan` construction carries a temporary `Value::Null` schema placeholder **Files**: `src/read_plan.rs:295,460-461,601` (three @@ -609,6 +687,34 @@ 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. +### 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", +"minimum": 1 }`, no maximum), `src/offset_map.rs` (`align_up`/ +`round_up`), `src/bast.rs:926` (`parse_align`) + +**Problem**: found during the H1 fix session (probe: a struct with +`align: 2^62` compiles in aligned mode and reports `total_size = +2^63`). The meta-schema accepts any `align >= 1`, and `align_up` +willingly rounds the running offset up by the full annotation — so a +one-field schema can declare a layout of exabyte scale. Unlike H1 this +does not crash: the allocation is offset *arithmetic*, not +`with_capacity`, and `validate_bytes` on the absurd layout fails +cleanly with a buffer-bounds `Access` error (probe-verified). Severity +is Nit because there is no abort and no unbounded memory *at read +time*; it is recorded because (a) `total_size` in that range is +meaningless output the consumer may act on, (b) the align-driven +stride is the one reachable path to `MAX_ARRAY_BYTES` (the H1 byte-cap +test in `offset_map` uses exactly this), and (c) the same unbounded +knob exists on `FieldDef.align`. A maximum align (e.g. 64 or 4096) in +the meta-schema and/or a clamp-with-error in `parse_align` closes it; +the honest layouts in the wild never need >64. + +**Fix**: add `"maximum"` to both `align` properties in +`bast_meta.rs`, or reject/clamp oversized values in `parse_align` +(with a clean `Schema` error). A locking test (align above the cap → +clean `Err`) mirrors the H1 test family. + --- ## What's Good @@ -648,9 +754,8 @@ Worth recording, because the findings shouldn't eclipse it: ## Recommended Order -1. **H1** — release-blocking. Smallest correct fix: drop the three - `Vec::with_capacity(count)`; add the compile-time cap + tests. - Isolate the reproducer (see the Methodology warning). +1. ~~**H1** — release-blocking~~ **resolved 2026-09-02** (with L1; + see the resolution block on the finding). 2. **H3** — needs a convention *decision* before code: write the addendum, enforce it, add L6's roundtrip test in the same session. 3. **H2** — shared walk guard (or the minimum doc note if the full @@ -661,8 +766,10 @@ Worth recording, because the findings shouldn't eclipse it: 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. -7. **L1–L6, N1** — opportunistic, folded into whichever session touches - the relevant file (L6 is the exception — it belongs with H3). +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 already + done with H1). ## Notes @@ -673,11 +780,18 @@ Worth recording, because the findings shouldn't eclipse it: If a reproducer test is wanted in-tree for H1/H2, it should be `#[ignore]`-gated with a comment pointing at the isolation requirements, or assert only the compile-time rejection (the safe - half of the fix) in the default suite. + half of the fix) in the default suite. The H1 resolution followed + the second option: in-tree tests assert only compile-time + rejections. - Coverage was measured with the default harness (`cargo llvm-cov --release`, summary + text). Numbers quoted are stable across two runs this session. - Severity here keys off AGENTS.md §3 (untrusted schema ⇒ handleable error) and the semver contract, not off effort: two of the three Highs are single-file, small-diff fixes; H3 is the only one that - needs a decision first. \ No newline at end of file + needs a decision first. +- The H1 fix session (2026-09-02) also confirmed the zero-progress + 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 diff --git a/src/bast.rs b/src/bast.rs index d50edfe..cba0e68 100644 --- a/src/bast.rs +++ b/src/bast.rs @@ -35,7 +35,9 @@ //! is used for any offset/count cast (AGENTS.md §4). use crate::error::AlkTypeError; -use crate::schema::{AlkTypeKind, Endian, VariableEncoding}; +use crate::schema::{ + AlkTypeKind, Endian, VariableEncoding, MAX_ARRAY_ELEMENTS, +}; use serde_json::Value; const DEFS_KEY: &str = "$defs"; @@ -851,6 +853,13 @@ impl BastArray { "bast: array at {path} has no `count` (variable-length arrays are not supported in v1, D-BAST-004)" )) })?; + if count > MAX_ARRAY_ELEMENTS { + return Err(AlkTypeError::Schema(format!( + "bast: array at {path} declares count {count}, which exceeds the \ + compile-time limit of {MAX_ARRAY_ELEMENTS} elements (untrusted schemas \ + must not be able to request unbounded per-element expansion)" + ))); + } Ok(Self { element: Box::new(element), count, @@ -1750,7 +1759,10 @@ mod tests { } #[test] - fn array_count_large_u64_parses_on_64bit() { + fn array_count_large_u64_rejected_by_compile_cap() { + // Pre-0.3.1 this parsed u64::MAX verbatim (H1: the count fed + // unbounded per-element expansion downstream). The cap now + // rejects it at parse, before any walker sees the array. let root = json!({ "$defs": { "S": { "kind": "struct", "fields": [ @@ -1758,14 +1770,12 @@ mod tests { ] } } }); - let d = BastDoc::new(&root, "S").expect("parses on usize>=u64 targets"); - let s = match d.root_def().kind() { - BastDefKind::Struct(s) => s, - _ => unreachable!(), - }; - match s.fields()[0].ty() { - BastType::Array(a) => assert_eq!(a.count(), usize::try_from(u64::MAX).unwrap()), - other => panic!("expected Array, got {other:?}"), + let err = BastDoc::new(&root, "S").unwrap_err(); + match err { + AlkTypeError::Schema(reason) => { + assert!(reason.contains("compile-time limit"), "reason: {reason}"); + } + other => panic!("expected Schema error, got {other:?}"), } } -} \ No newline at end of file +} diff --git a/src/layout_builder.rs b/src/layout_builder.rs index e112296..c9a403d 100644 --- a/src/layout_builder.rs +++ b/src/layout_builder.rs @@ -44,7 +44,9 @@ use crate::bast::{ BastUnion, }; use crate::error::AlkTypeError; -use crate::schema::{AlkTypeKind, Endian, DISCRIMINATOR_PATH, U32_SIZE}; +use crate::schema::{ + AlkTypeKind, Endian, DISCRIMINATOR_PATH, U32_SIZE, MAX_ARRAY_BYTES, +}; use serde_json::Value; use std::collections::HashMap; @@ -350,6 +352,21 @@ impl<'d> BuildCtx<'d> { reason: format!("element kind {elem_kind} has no fixed size"), })?; let count = array.count(); + let array_bytes = count + .checked_mul(elem_size) + .ok_or_else(|| AlkTypeError::Offset { + field_path: field_path.to_string(), + reason: format!("array size {count} × {elem_size} overflows usize"), + })?; + if array_bytes > MAX_ARRAY_BYTES { + return Err(AlkTypeError::Offset { + field_path: field_path.to_string(), + reason: format!( + "array size {count} × {elem_size} = {array_bytes} bytes exceeds the \ + compile-time limit of {MAX_ARRAY_BYTES} bytes" + ), + }); + } let start = *offset; for i in 0..count { @@ -896,6 +913,51 @@ mod tests { assert_eq!(layout.total_size(), 12); } + // ----- H1: array caps bound untrusted schemas at build time -------- + + #[test] + fn h1_array_count_above_cap_rejected_at_parse() { + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "fields": [ + { "name": "vals", "kind": { "kind": "array", "element": "uint8", "count": 2000000000 } } + ] + } + } + }); + let err = LayoutBuilder::new(&root, "S").unwrap_err(); + match err { + AlkTypeError::Schema(reason) => { + assert!(reason.contains("compile-time limit"), "reason: {reason}"); + } + other => panic!("expected Schema error, got {other:?}"), + } + } + + #[test] + fn h1_array_bytes_above_cap_rejected_at_build_is_defense_in_depth() { + // The byte cap cannot be reached through the public API with the + // current caps (packed mode has no alignment: max fixed element + // is 8 bytes, count <= 2^16, product <= 2^19 << 2^26) — the + // check exists to hold if the element cap or the fixed-size kind + // set ever grows. The reachable byte-cap path is exercised in + // offset_map (aligned mode, align-driven stride). + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "fields": [ + { "name": "vals", "kind": { "kind": "array", "element": "uint64", "count": 65536 } } + ] + } + } + }); + let layout = build(&root, "S", &var_sizes(&[])); + assert_eq!(layout.total_size(), 8 * 65536); + } + #[test] fn array_fixed_count_after_preceding_field() { let root = json!({ diff --git a/src/materialize.rs b/src/materialize.rs index d415ce3..a180f92 100644 --- a/src/materialize.rs +++ b/src/materialize.rs @@ -245,7 +245,7 @@ fn materialize_plan_array( ))) } }; - let mut arr = Vec::with_capacity(count); + let mut arr = Vec::new(); for i in 0..count { let path = format!("{field_path}[{i}]"); let value = materialize_plan_composite(element, &path, endian, buffer, offset)?; @@ -634,10 +634,11 @@ fn materialize_array_packed( ) -> Result { let element_ty = array.element(); let count = array.count(); - let mut arr = Vec::with_capacity(count); + let mut arr = Vec::new(); for i in 0..count { let path = format!("{field_path}[{i}]"); let resolved_elem = doc.resolve_typeref(element_ty)?; + let before = *offset; let value = materialize_typeref_packed( doc, &resolved_elem, @@ -646,6 +647,15 @@ fn materialize_array_packed( buffer, offset, )?; + if *offset == before { + return Err(AlkTypeError::Access { + field_path: path, + reason: format!( + "array element {i} consumed 0 bytes; a zero-size element makes the \ + declared count unbounded on the wire" + ), + }); + } arr.push(value); } Ok(Value::Array(arr)) @@ -921,7 +931,7 @@ fn materialize_array_aligned( let element_ty = array.element(); let resolved_elem = doc.resolve_typeref(element_ty)?; let count = array.count(); - let mut arr = Vec::with_capacity(count); + let mut arr = Vec::new(); for i in 0..count { let elem_path = format!("{field_path}[{i}]"); let entry = offset_map.get(&elem_path).ok_or_else(|| AlkTypeError::Offset { diff --git a/src/offset_map.rs b/src/offset_map.rs index 5a79280..a97fa11 100644 --- a/src/offset_map.rs +++ b/src/offset_map.rs @@ -16,7 +16,7 @@ use crate::bast::{ BastArray, BastDefKind, BastDoc, BastField, BastStruct, BastType, }; use crate::error::AlkTypeError; -use crate::schema::{AlkTypeKind, Endian, VariableEncoding}; +use crate::schema::{AlkTypeKind, Endian, VariableEncoding, MAX_ARRAY_BYTES}; /// A byte range within a buffer. /// @@ -415,6 +415,21 @@ impl<'d> ComputeCtx<'d> { let elem_align = element_alignment(&resolved_elem, struct_default_align, elem_natural); let stride = round_up(elem_size, elem_align); let count = array.count(); + let array_bytes = count + .checked_mul(stride) + .ok_or_else(|| AlkTypeError::Offset { + field_path: field_path.to_string(), + reason: format!("array size {count} × stride {stride} overflows usize"), + })?; + if array_bytes > MAX_ARRAY_BYTES { + return Err(AlkTypeError::Offset { + field_path: field_path.to_string(), + reason: format!( + "array size {count} × stride {stride} = {array_bytes} bytes exceeds the \ + compile-time limit of {MAX_ARRAY_BYTES} bytes" + ), + }); + } let array_align = field_alignment(field, struct_default_align, elem_align); @@ -682,6 +697,58 @@ mod tests { assert_eq!(m.total_size(), 12); } + // ----- H1: array caps bound untrusted schemas at compute time ------ + + #[test] + fn h1_array_count_above_cap_rejected_at_parse() { + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "fields": [ + { "name": "vals", "kind": { "kind": "array", "element": "uint8", "count": 2000000000 } } + ] + } + } + }); + let err = BastDoc::new(&root, "S").unwrap_err(); + match err { + AlkTypeError::Schema(reason) => { + assert!(reason.contains("compile-time limit"), "reason: {reason}"); + } + other => panic!("expected Schema error, got {other:?}"), + } + } + + #[test] + fn h1_array_bytes_above_cap_rejected_at_compute() { + // 65536 elements × stride 2^20 (a u8 array in a struct with + // align 2^20) = 2^36 > 2^26 — hits the byte cap while staying + // under the element cap. (A struct/array element would be + // rejected earlier as variable-length, OQ-001, so the reachable + // way over the byte cap is a huge stride, not a huge element.) + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "align": 1048576u64, + "fields": [ + { "name": "vals", "kind": { "kind": "array", "element": "uint8", "count": 65536 } } + ] + } + } + }); + let doc = BastDoc::new(&root, "S").expect("bast doc (under element cap)"); + let err = OffsetMap::compute(&doc).unwrap_err(); + match err { + AlkTypeError::Offset { field_path, reason } => { + assert_eq!(field_path, "vals"); + assert!(reason.contains("exceeds the compile-time limit"), "reason: {reason}"); + } + other => panic!("expected Offset error, got {other:?}"), + } + } + #[test] fn variable_string_length_prefix_at_known_offset() { let root = json!({ diff --git a/src/read_plan.rs b/src/read_plan.rs index 0c45cf0..00e90e6 100644 --- a/src/read_plan.rs +++ b/src/read_plan.rs @@ -41,7 +41,9 @@ use crate::bast::{ BastType, BastUnion, }; use crate::error::AlkTypeError; -use crate::schema::{AlkTypeKind, Endian, VariableEncoding}; +use crate::schema::{ + AlkTypeKind, Endian, VariableEncoding, MAX_ARRAY_BYTES, +}; use serde_json::Value; use std::collections::{BTreeMap, BTreeSet}; use std::sync::Arc; @@ -544,10 +546,22 @@ fn compile_array( let element_path = format!("{path}.element"); let (kind, body) = compile_typeref(doc, a.element(), container_endian, &element_path, depth, seen)?; let element = wrap_leaf(kind, body, container_endian, &element_path)?; - let element_stride = fixed_composite_size(&element).unwrap_or_default(); + let element_stride = fixed_composite_size(&element)?.unwrap_or(0); + let count = a.count(); + let total = count.checked_mul(element_stride).ok_or_else(|| { + AlkTypeError::Schema(format!( + "read_plan: array size {count} × {element_stride} overflows usize at {path}" + )) + })?; + if total > MAX_ARRAY_BYTES { + return Err(AlkTypeError::Schema(format!( + "read_plan: array at {path} computes to {count} × {element_stride} = {total} bytes, \ + which exceeds the compile-time limit of {MAX_ARRAY_BYTES} bytes" + ))); + } Ok(CompositePlan::Array { element: Box::new(element), - count: a.count(), + count, element_stride, }) } @@ -613,44 +627,82 @@ fn wrap_leaf( /// contribute their fixed size; structs sum their fields; nested arrays /// with fixed elements contribute `count × stride`; unions, records, /// and variable-length primitives make the whole node variable. -fn fixed_composite_size(body: &CompositePlan) -> Option { +/// +/// Returns `Err` only on arithmetic overflow while summing struct field +/// sizes or array products — after the array caps ([`MAX_ARRAY_BYTES`], +/// [`MAX_ARRAY_ELEMENTS`]) no honest schema can reach those arms, and +/// treating overflow as "variable-length" (the pre-0.3.1 +/// `unwrap_or_default` behavior) would silently mis-plan the wire. +fn fixed_composite_size(body: &CompositePlan) -> Result, AlkTypeError> { match body { CompositePlan::Struct(plan) => fixed_plan_size(plan), - CompositePlan::Union { .. } | CompositePlan::Record { .. } => None, + CompositePlan::Union { .. } | CompositePlan::Record { .. } => Ok(None), CompositePlan::Array { count, element_stride, .. } => { if *element_stride == 0 { - None + Ok(None) } else { - count.checked_mul(*element_stride) + match count.checked_mul(*element_stride) { + Some(total) => Ok(Some(total)), + None => Err(AlkTypeError::Schema( + "internal: array size product overflowed while sizing a fixed-stride \ + composite (array caps should have rejected this schema earlier)" + .to_string(), + )), + } } } } } -fn fixed_plan_size(plan: &ReadPlan) -> Option { +fn fixed_plan_size(plan: &ReadPlan) -> Result, AlkTypeError> { let mut total = 0usize; for field in plan.fields() { let size = match field.kind() { - ReadKind::Primitive(k) if k.is_fixed_size() => k.type_size()?, - ReadKind::Enum => AlkTypeKind::Enum.type_size()?, - ReadKind::Struct | ReadKind::Array => { - fixed_composite_size(field.body().as_ref()?)? + ReadKind::Primitive(k) if k.is_fixed_size() => { + k.type_size().ok_or_else(|| { + AlkTypeError::Schema(format!( + "internal: fixed kind {k} has no size while sizing a plan" + )) + })? } - ReadKind::Union | ReadKind::Record => return None, - ReadKind::Primitive(_) => return None, + ReadKind::Enum => AlkTypeKind::Enum.type_size().ok_or_else(|| { + AlkTypeError::Schema("internal: enum has no size while sizing a plan".to_string()) + })?, + ReadKind::Struct | ReadKind::Array => { + match fixed_composite_size( + field + .body() + .as_ref() + .ok_or_else(|| AlkTypeError::Schema(format!( + "internal: composite field {} has no body while sizing", + field.name() + )))?, + )? { + Some(size) => size, + None => return Ok(None), + } + } + ReadKind::Union | ReadKind::Record => return Ok(None), + ReadKind::Primitive(_) => return Ok(None), }; - total = total.checked_add(size)?; + total = total.checked_add(size).ok_or_else(|| { + AlkTypeError::Schema( + "internal: struct field sizes overflowed usize while sizing a plan".to_string(), + ) + })?; } - Some(total) + Ok(Some(total)) } #[cfg(test)] mod tests { use super::*; + use crate::engine::{AlkTypeEngine, LayoutMode}; + use crate::schema::MAX_ARRAY_ELEMENTS; use serde_json::json; const LE: Endian = Endian::Little; @@ -926,6 +978,72 @@ mod tests { } } + // ----- H1: array caps bound untrusted schemas at compile time ------- + + fn array_root(count: serde_json::Value, element: serde_json::Value) -> Value { + json!({ "$defs": { "S": { "kind": "struct", "fields": [ + { "name": "vals", "kind": { "kind": "array", "element": element, "count": count } } + ]}}}) + } + + #[test] + fn h1_array_count_above_cap_rejected_at_compile() { + let root = array_root(serde_json::json!(2_000_000_000u64), serde_json::json!("uint8")); + let err = ReadPlan::compile(&root, "S").unwrap_err(); + match err { + AlkTypeError::Schema(reason) => { + assert!( + reason.contains("compile-time limit"), + "reason: {reason}" + ); + } + other => panic!("expected Schema error, got {other:?}"), + } + } + + #[test] + fn h1_array_count_at_cap_compiles() { + let root = array_root( + serde_json::json!(MAX_ARRAY_ELEMENTS), + serde_json::json!("uint8"), + ); + let p = plan(&root, "S"); + match p.fields()[0].body().expect("array body") { + CompositePlan::Array { count, .. } => assert_eq!(*count, MAX_ARRAY_ELEMENTS), + other => panic!("expected Array body, got {other:?}"), + } + } + + #[test] + fn h1_array_bytes_above_cap_rejected_at_compile() { + // 65536 elements × 1032-byte stride (129 × u64 inner array) = + // 67,633,152 > 2^26 — hits the byte cap while staying under the + // element cap. + let root = array_root( + serde_json::json!(MAX_ARRAY_ELEMENTS), + serde_json::json!({ "kind": "array", "element": "uint64", "count": 129 }), + ); + let err = ReadPlan::compile(&root, "S").unwrap_err(); + match err { + AlkTypeError::Schema(reason) => { + assert!( + reason.contains("exceeds the compile-time limit"), + "reason: {reason}" + ); + } + other => panic!("expected Schema error, got {other:?}"), + } + } + + #[test] + fn h1_huge_count_short_buffer_validate_is_clean_error() { + let root = array_root(serde_json::json!(1024), serde_json::json!("uint8")); + let engine = + AlkTypeEngine::compile(&root, "S", LayoutMode::Packed, None).expect("compile"); + let err = engine.validate_bytes(&[0u8; 4]).unwrap_err(); + assert!(matches!(err, AlkTypeError::Access { .. }), "got {err:?}"); + } + #[test] fn cov_record() { let root = json!({ "$defs": { "S": { "kind": "struct", "fields": [ diff --git a/src/schema.rs b/src/schema.rs index 761fdfa..901f76e 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -16,6 +16,20 @@ use std::fmt; pub(crate) const U32_SIZE: usize = 4; pub(crate) const DISCRIMINATOR_PATH: &str = "__discriminator"; +/// Maximum declared array element count. Schemas are untrusted input +/// (AGENTS.md §3): every layout walker expands `count` into per-element +/// work (offset-map/layout entries, per-element reads), so an unbounded +/// count is a memory/CPU denial-of-service. The cap applies at parse +/// time, before any walker sees the array; 2^16 elements bounds the +/// per-array compile-time entry cost at ~10 MB. +pub(crate) const MAX_ARRAY_ELEMENTS: usize = 1 << 16; + +/// Maximum computed byte size of a single array (`count × element +/// stride`). Bounds the arithmetic product independently of +/// [`MAX_ARRAY_ELEMENTS`] so multi-megabyte strides cannot combine with +/// a legal count into an unbounded layout request. +pub(crate) const MAX_ARRAY_BYTES: usize = 1 << 26; + /// The 18 BAST kinds recognized by the engine. /// /// Each variant corresponds to a lowercase BAST kind string diff --git a/src/sequential_reader.rs b/src/sequential_reader.rs index 484b5b6..7d07617 100644 --- a/src/sequential_reader.rs +++ b/src/sequential_reader.rs @@ -765,6 +765,15 @@ fn plan_walk_variable_array_size( reason: format!("array element walked backwards: {position} → {new_position}"), }); } + if new_position == position { + return Err(AlkTypeError::Access { + field_path: element_path, + reason: format!( + "array element {i} consumed 0 bytes; a zero-size element makes the \ + declared count unbounded on the wire" + ), + }); + } position = new_position; } Ok(position - start)