Fix L2: replace unreachable! with Err for untrusted-schema safety
The immediate downstream consumer (alkcall) accepts schemas from
arbitrary internet peers in its hub/spoke topology. A panic is the
wrong failure mode for a malicious or unsupported schema - an Err
the caller can handle is correct.
Three sites converted:
- offset_map.rs: _ => Err(Offset { unsupported AlkType kind for
aligned offset computation })
- layout_builder.rs: _ => Err(Offset { unsupported AlkType kind
for packed layout computation })
- materialize.rs: _ => Err(Schema { internal: union discriminator
type N is not a supported byte discriminator })
The materialize.rs site uses Schema (not Access) because a wrong
disc_type is a schema-authoring bug (parse_discriminator should have
caught it), not a buffer-access error. The sibling sites in
sequential_reader.rs and tunion.rs already returned Err(Schema) -
only materialize.rs was the holdout.
Note: the k if k.is_fixed_size() guard in offset_map and
layout_builder means the compiler cannot enforce exhaustiveness at
compile time. Converting _ from unreachable! to Err is the runtime
mitigation. A future refactor could list all fixed-size kinds
explicitly to restore compile-time checking. Deferred to a separate
cleanup pass.
Verified: no unreachable! remains in production code (the one
remaining hit at offset_map.rs:688 is inside a #[test] fn).
cargo test --release: 396 tests pass, 0 failures
cargo clippy --all-targets -- -D warnings: clean
cargo build --target wasm32-unknown-unknown --release: clean (prior)
This commit is contained in:
@@ -1,5 +1,5 @@
|
||||
---
|
||||
status: resolved (M1, M2, L1, L3); open (L2, N1, N2)
|
||||
status: resolved (M1, M2, L1, L2, L3); open (N1, N2)
|
||||
last_updated: 2026-08-11
|
||||
reviewed_artifacts:
|
||||
- src/lib.rs
|
||||
@@ -516,12 +516,6 @@ and `source_returns_some_for_validation_variant` (new). ~6 lines +
|
||||
|
||||
### Deferred
|
||||
|
||||
- **L2** (`unreachable!` → `Err`): the three `unreachable!` sites are
|
||||
genuinely unreachable today. Converting them to `Err` is defense-in-
|
||||
depth against a future `AlkTypeKind` variant addition or a
|
||||
`parse_discriminator` logic bug. Deferred until the "load untrusted
|
||||
schemas" use case is on the roadmap — until then, the exhaustiveness
|
||||
check is the safety net.
|
||||
- **N1** (non-strict `is_rfc3339_timestamp`): documented as "simple"
|
||||
in the existing doc comment. A strict implementation would add a
|
||||
`chrono` or `time` dependency, not worth it for 0.1.0. Will add an
|
||||
@@ -529,6 +523,50 @@ and `source_returns_some_for_validation_variant` (new). ~6 lines +
|
||||
- **N2** (`FieldValue::Bytes` for `Record`): API asymmetry, not a
|
||||
bug. Revisit if the alkcall consumer finds it awkward.
|
||||
|
||||
After M1, M2, L1, and L3, the remaining open findings (L2, N1, N2)
|
||||
are all deferrable. The crate is ready for the pre-publish docs sweep
|
||||
(README, inline doc cleanup for docs.rs) and the final sanity check.
|
||||
### L2 (`unreachable!` → `Err`) — resolved (follow-up)
|
||||
|
||||
The three `unreachable!` sites were originally deferred. After
|
||||
discussion with the publisher, the immediate downstream consumer
|
||||
(`alkcall`) accepts schemas from arbitrary internet peers in its
|
||||
hub/spoke topology — there is no way to guarantee the remote side
|
||||
won't produce a malicious or unsupported schema definition. A panic
|
||||
is the wrong failure mode for that threat model; an `Err` the caller
|
||||
can handle is correct.
|
||||
|
||||
All three sites converted to `Err(AlkTypeError::Offset/Schema(...))`:
|
||||
|
||||
- `offset_map.rs:277` — `_ => Err(Offset { ... "unsupported AlkType
|
||||
kind for aligned offset computation" })`
|
||||
- `layout_builder.rs:285` — `_ => Err(Offset { ... "unsupported
|
||||
AlkType kind for packed layout computation" })`
|
||||
- `materialize.rs:324` — `_ => return Err(Schema(... "internal: union
|
||||
discriminator type N is not a supported byte discriminator
|
||||
(parse_discriminator should have rejected this)"))`
|
||||
|
||||
The `materialize.rs` site uses `Schema` rather than `Access` because
|
||||
a wrong `disc_type` is a schema-authoring bug (the `parse_discriminator`
|
||||
validator should have caught it earlier), not a buffer-access error.
|
||||
The `offset_map` and `layout_builder` sites use `Offset` to match
|
||||
their surrounding error variants. The sibling sites in
|
||||
`sequential_reader.rs::read_byte_discriminator` (line 535) and
|
||||
`tunion.rs::read_byte_discriminator` (line 70) already returned
|
||||
`Err(Schema(...))` — only `materialize.rs` was the holdout.
|
||||
|
||||
Note: the `k if k.is_fixed_size()` guard pattern in `offset_map` and
|
||||
`layout_builder` means the compiler cannot enforce exhaustiveness on
|
||||
these match arms (the guard catches new variants at runtime, not
|
||||
compile time). Converting the `_` arm from `unreachable!` to `Err`
|
||||
is the defense-in-depth mitigation: a new `AlkTypeKind` variant added
|
||||
without updating these matches produces a handled error, not a panic.
|
||||
A future refactor could remove the `is_fixed_size()` guard and list
|
||||
all fixed-size kinds explicitly — that would restore compile-time
|
||||
exhaustiveness checking. Deferred to a separate cleanup pass.
|
||||
|
||||
Verified: no `unreachable!` remains in production code (the one
|
||||
remaining hit at `offset_map.rs:688` is inside a `#[test]` fn,
|
||||
guarded by `assert!(matches!(...))` on the line above).
|
||||
|
||||
After M1, M2, L1, L2, and L3, the remaining open findings (N1, N2)
|
||||
are both deferrable to the docs sweep. The crate is ready for the
|
||||
pre-publish docs sweep (README, inline doc cleanup for docs.rs) and
|
||||
the final sanity check.
|
||||
@@ -282,7 +282,10 @@ impl<'a> BuildCtx<'a> {
|
||||
self.push(field_path, start, size, k);
|
||||
Ok(())
|
||||
}
|
||||
_ => unreachable!("all AlkTypeKind variants are covered above"),
|
||||
other => Err(AlkTypeError::Offset {
|
||||
field_path: field_path.to_string(),
|
||||
reason: format!("unsupported AlkType kind {other} for packed layout computation"),
|
||||
}),
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -329,7 +329,12 @@ fn materialize_union_packed(
|
||||
AlkTypeKind::Uint32 => {
|
||||
data_access::read_u32(buffer, disc_abs_offset, field_path, endian)?
|
||||
}
|
||||
_ => unreachable!("disc_type restricted by parse_discriminator"),
|
||||
other => {
|
||||
return Err(AlkTypeError::Schema(format!(
|
||||
"internal: union discriminator type {other} is not a supported byte \
|
||||
discriminator (parse_discriminator should have rejected this)"
|
||||
)));
|
||||
}
|
||||
};
|
||||
let mapping = field_schema.get("mapping").and_then(Value::as_object).ok_or_else(|| {
|
||||
AlkTypeError::Schema(format!(
|
||||
|
||||
@@ -274,7 +274,10 @@ impl<'a> ComputeCtx<'a> {
|
||||
k if k.is_fixed_size() => {
|
||||
self.compute_fixed_field(k, field_schema, field_path, struct_default_align)
|
||||
}
|
||||
_ => unreachable!("all AlkTypeKind variants are covered above"),
|
||||
other => Err(AlkTypeError::Offset {
|
||||
field_path: field_path.to_string(),
|
||||
reason: format!("unsupported AlkType kind {other} for aligned offset computation"),
|
||||
}),
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user