From 0bc5a541ac8030fc30f0ec91a170a9d1645d7910 Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Wed, 2 Sep 2026 19:46:51 +0000 Subject: [PATCH] Resolve N2: bound align annotations at 4096 (review #006) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit align: 2^62 compiled and reported total_size = 2^63 — meaningless layout output the consumer may act on, and the reachable path to the MAX_ARRAY_BYTES cap used exactly this knob. - MAX_ALIGN = 4096 (page granularity) in schema.rs, documented with the probe arithmetic - parse_align returns Result and rejects over-cap values with a clean Schema error naming path/value/maximum — a silent clamp was rejected (it would change layout semantics without telling the consumer); both call sites thread the path, so standalone BastDoc::new (which never runs the meta-schema) is covered - Meta-schema: "maximum": 4096 on StructDef.align and FieldDef.align — the published alk.dev/bast/v1/schema contract now matches the parser - H1's byte-cap test retuned to align 4096 x count 2^16 = 2^28 > 2^26 (the byte cap stays reachable under the new align cap) Tests: 3 new (struct align above cap, field align above cap, align at cap accepted). 511 tests green, clippy -D warnings clean, wasm32 build green, cargo doc zero warnings. --- docs/reviews/006-implementation-review-030.md | 46 +++++++++++-- src/bast.rs | 37 ++++++++--- src/bast_meta.rs | 4 +- src/offset_map.rs | 65 +++++++++++++++++-- src/schema.rs | 8 +++ 5 files changed, 139 insertions(+), 21 deletions(-) diff --git a/docs/reviews/006-implementation-review-030.md b/docs/reviews/006-implementation-review-030.md index fa7ff8f..751b2a5 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 resolved 2026-09-02) +status: in-progress (H1, L1, H3, L5, L6, H2, M1, M2, M3, L2, L3, N2 resolved 2026-09-02) last_updated: 2026-09-02 reviewed_artifacts: - src/read_plan.rs @@ -98,10 +98,11 @@ sub-types, `fingerprint()` methods; `Hash` on `Endian`/ | 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) | open | +| Nit | 2 (N1, N2) | N2 resolved 2026-09-02; N1 open | -All three Highs, three of four Mediums, and five of six Lows are -resolved. Remaining: M4 (ongoing per-fix coverage posture), L4, N1, N2. +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. The three Highs are adversarial-input crashes (H1, H2) and a cross-consumer wire-layout convention gap (H3) — all three are the @@ -141,6 +142,13 @@ them became materially easier to hit with the 0.3.0 surface. 0.3.0 compiled-form files already in play) — see the resolution blocks on each finding. 508 tests green, clippy `-D warnings` clean, wasm build green, `cargo doc --no-deps` zero warnings. +- **N2 (2026-09-02):** resolved — see the "Resolution (2026-09-02)" + block on the finding. `MAX_ALIGN = 4096` enforced in `parse_align` + (clean `Schema` error, standalone-safe) and in the meta-schema + (`"maximum": 4096` on both align properties); H1's byte-cap test + retuned to the new legal maximum. 511 tests green, clippy + `-D warnings` clean, wasm build green, `cargo doc --no-deps` zero + warnings. --- @@ -954,6 +962,33 @@ the honest layouts in the wild never need >64. (with a clean `Schema` error). A locking test (align above the cap → clean `Err`) mirrors the H1 test family. +**Resolution (2026-09-02):** both layers, cap = 4096 (page granularity; +the finding's suggestion): + +1. **`MAX_ALIGN = 4096`** added to `schema.rs` (documented with the + N2 probe arithmetic: `align: 2^62` → `total_size = 2^63`). +2. **`parse_align` now returns `Result>`** and rejects + over-cap values with a clean `Schema` error naming the path, the + value, and the maximum — a clamp-with-silent-drop was considered + and rejected: it would change layout semantics without telling the + consumer (AGENTS.md §3 wants a handleable error, not a surprise). + Both call sites (`BastStruct::parse`, `BastField::parse`) thread the + path. This closes the standalone `BastDoc::new` path, which never + runs the meta-schema. +3. **Meta-schema `"maximum": 4096`** added to both `align` properties + (`StructDef.align`, `FieldDef.align`) — the published + `https://alk.dev/bast/v1/schema` contract now matches the parser. +4. **H1 byte-cap test retuned**: the old fixture used `align: 2^20` to + reach `MAX_ARRAY_BYTES`; it now uses `align: 4096` × count 2^16 = + 2^28 > 2^26 — the byte cap stays reachable under the new align cap + (the finding's point (b) remains testable). + +Tests: `n2_struct_align_above_cap_rejected_at_parse` (message names +align + maximum), `n2_field_align_above_cap_rejected_at_parse`, +`n2_align_at_cap_accepted` (boundary: align 4096 accepted, total_size +4096). Verified: 511 tests green, clippy `-D warnings` clean, wasm +build green, `cargo doc --no-deps` zero warnings. + --- ## What's Good @@ -1011,7 +1046,8 @@ Worth recording, because the findings shouldn't eclipse it: 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). + with H1; L5/L6 done with H3; L2/L3 done with the M-fix session; + N2 done in the M-fix session's tail). ## Notes diff --git a/src/bast.rs b/src/bast.rs index b344e78..3a0f82f 100644 --- a/src/bast.rs +++ b/src/bast.rs @@ -36,7 +36,7 @@ use crate::error::AlkTypeError; use crate::schema::{ - AlkTypeKind, Endian, VariableEncoding, MAX_ARRAY_ELEMENTS, + AlkTypeKind, Endian, VariableEncoding, MAX_ALIGN, MAX_ARRAY_ELEMENTS, }; use serde_json::Value; @@ -308,7 +308,7 @@ impl BastStruct { fn parse(node: &Value, path: &str, doc_root: &Value) -> Result { let endian = parse_endian_opt(node).unwrap_or(Endian::Little); - let align = parse_align(node); + let align = parse_align(node, path)?; let raw_fields = node .get("fields") .and_then(Value::as_array) @@ -427,7 +427,7 @@ impl BastField { })?; let ty = BastType::parse(raw_kind, path, doc_root)?; let endian = parse_endian_opt(node); - let align = parse_align(node); + let align = parse_align(node, path)?; let encoding = parse_encoding(node); let max_length = parse_max_length(node); Ok(Self { @@ -993,11 +993,32 @@ fn parse_endian_opt(node: &Value) -> Option { } } -fn parse_align(node: &Value) -> Option { - node.as_object() - .and_then(|o| o.get("align")) - .and_then(Value::as_u64) - .and_then(|n| usize::try_from(n).ok()) +/// Parse a struct- or field-level `align` annotation. Over-sized values +/// are a clean `Schema` error (review #006 N2: an unbounded align let a +/// one-field schema declare an exabyte-scale layout — `align_up` rounds +/// the running offset by the full annotation). +fn parse_align(node: &Value, path: &str) -> Result, AlkTypeError> { + let raw = match node.as_object().and_then(|o| o.get("align")) { + None | Some(Value::Null) => return Ok(None), + Some(v) => v, + }; + let n = raw.as_u64().ok_or_else(|| { + AlkTypeError::Schema(format!( + "bast: align at {path} is not a non-negative integer" + )) + })?; + let n = usize::try_from(n).map_err(|_| { + AlkTypeError::Schema(format!( + "bast: align at {path} (= {n}) overflows usize" + )) + })?; + if n > MAX_ALIGN { + return Err(AlkTypeError::Schema(format!( + "bast: align at {path} (= {n}) exceeds the maximum of {MAX_ALIGN} \ + (review #006 N2: honest layouts never need more than page granularity)" + ))); + } + Ok(Some(n)) } fn parse_max_length(node: &Value) -> Option { diff --git a/src/bast_meta.rs b/src/bast_meta.rs index 000242a..c5a71a6 100644 --- a/src/bast_meta.rs +++ b/src/bast_meta.rs @@ -61,7 +61,7 @@ pub static BAST_META_SCHEMA: LazyLock = LazyLock::new(|| { "properties": { "kind": { "const": "struct" }, "endian": { "enum": ["little", "big"] }, - "align": { "type": "integer", "minimum": 1 }, + "align": { "type": "integer", "minimum": 1, "maximum": 4096 }, "fields": { "type": "array", "items": { "$ref": "#/$defs/FieldDef" } @@ -76,7 +76,7 @@ pub static BAST_META_SCHEMA: LazyLock = LazyLock::new(|| { "name": { "type": "string", "pattern": "^[a-zA-Z_][a-zA-Z0-9_]*$" }, "kind": { "$ref": "#/$defs/TypeRef" }, "endian": { "enum": ["little", "big"] }, - "align": { "type": "integer", "minimum": 1 }, + "align": { "type": "integer", "minimum": 1, "maximum": 4096 }, "encoding": { "enum": ["length-prefixed", "offset-indirect"] }, "maxLength": { "type": "integer", "minimum": 0 } }, diff --git a/src/offset_map.rs b/src/offset_map.rs index 340f8c4..aa0d89b 100644 --- a/src/offset_map.rs +++ b/src/offset_map.rs @@ -710,6 +710,58 @@ mod tests { // ----- H1: array caps bound untrusted schemas at compute time ------ + #[test] + fn n2_struct_align_above_cap_rejected_at_parse() { + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "align": 4097u64, + "fields": [ { "name": "v", "kind": "uint8" } ] + } + } + }); + let err = BastDoc::new(&root, "S").unwrap_err(); + match err { + AlkTypeError::Schema(reason) => { + assert!(reason.contains("align"), "reason: {reason}"); + assert!(reason.contains("maximum"), "reason: {reason}"); + } + other => panic!("expected Schema error, got {other:?}"), + } + } + + #[test] + fn n2_field_align_above_cap_rejected_at_parse() { + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "fields": [ + { "name": "v", "kind": "uint8", "align": 8192 } + ] + } + } + }); + let err = BastDoc::new(&root, "S").unwrap_err(); + assert!(matches!(err, AlkTypeError::Schema(_)), "got {err:?}"); + } + + #[test] + fn n2_align_at_cap_accepted() { + let root = json!({ + "$defs": { + "S": { + "kind": "struct", + "align": 4096u64, + "fields": [ { "name": "v", "kind": "uint8" } ] + } + } + }); + let m = map(&root, "S"); + assert_eq!(m.total_size(), 4096); + } + #[test] fn h1_array_count_above_cap_rejected_at_parse() { let root = json!({ @@ -733,16 +785,17 @@ mod tests { #[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.) + // 65536 elements × stride 4096 (a u8 array in a struct with the + // maximum legal align, N2) = 2^28 > 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 large stride, not a huge + // element.) let root = json!({ "$defs": { "S": { "kind": "struct", - "align": 1048576u64, + "align": 4096u64, "fields": [ { "name": "vals", "kind": { "kind": "array", "element": "uint8", "count": 65536 } } ] diff --git a/src/schema.rs b/src/schema.rs index 901f76e..52d58d2 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -30,6 +30,14 @@ pub(crate) const MAX_ARRAY_ELEMENTS: usize = 1 << 16; /// a legal count into an unbounded layout request. pub(crate) const MAX_ARRAY_BYTES: usize = 1 << 26; +/// Maximum declared `align` annotation (struct- or field-level). +/// Schemas are untrusted input (AGENTS.md §3): `align_up` rounds the +/// running offset by the full annotation, so an unbounded align lets a +/// one-field schema declare an exabyte-scale layout (`total_size = 2^63` +/// from `align: 2^62` — review #006 N2 probe). Honest layouts never +/// need more than page granularity (4096). +pub(crate) const MAX_ALIGN: usize = 4096; + /// The 18 BAST kinds recognized by the engine. /// /// Each variant corresponds to a lowercase BAST kind string