diff --git a/crates/xrpl-wasm-vm/src/abi.rs b/crates/xrpl-wasm-vm/src/abi.rs index 94b3ad6540..db6db25fda 100644 --- a/crates/xrpl-wasm-vm/src/abi.rs +++ b/crates/xrpl-wasm-vm/src/abi.rs @@ -1,3 +1,4 @@ +use crate::region::Region; use crate::vm::{MAX_FIELD_BYTES, VmState}; use wasmi::{Caller, Memory}; use xrpl_host_functions::{HostError, HostFunctionSpec, HostFunctions, HostResult}; @@ -67,25 +68,14 @@ fn memory(caller: &Caller<'_, VmState<'_>>) -> Result { caller.data().memory.ok_or(HostError::NoMemExported) } -/// Validate `[ptr, ptr + len)` against `data` and return that slice of it. -pub(crate) fn region(data: &[u8], ptr: i32, len: i32) -> HostResult<&[u8]> { - let (Ok(ptr), Ok(len)) = (usize::try_from(ptr), usize::try_from(len)) else { - return Err(HostError::InvalidParams); - }; - if len > MAX_FIELD_BYTES { - return Err(HostError::DataFieldTooLarge); - } - let end = ptr.checked_add(len).ok_or(HostError::PointerOutOfBounds)?; - data.get(ptr..end).ok_or(HostError::PointerOutOfBounds) -} - +/// [`Region::read`] of the guest's memory, for a call that reads and writes nothing +/// back (`trace`, `trace_num`). pub(crate) fn read_borrowed<'a>( caller: &'a Caller<'_, VmState<'_>>, - ptr: i32, - len: i32, + input: Region, ) -> HostResult<&'a [u8]> { let mem = memory(caller)?; - region(mem.data(caller), ptr, len) + input.read(mem.data(caller)) } /// Service a call whose answer is bytes, written straight into the guest's output @@ -97,27 +87,24 @@ pub(crate) fn read_borrowed<'a>( /// cap, and both checks below are reachable. pub(crate) fn write_into( caller: &mut Caller<'_, VmState<'_>>, - dst: i32, - cap: i32, + out: Region, fill: impl FnOnce(&dyn HostFunctions, &mut [u8]) -> HostResult, ) -> HostResult { - let (Ok(dst), Ok(cap)) = (usize::try_from(dst), usize::try_from(cap)) else { - return Err(HostError::InvalidParams); - }; + let range = out.range()?; + let cap = range.len(); let mem = memory(caller)?; let host: &dyn HostFunctions = caller.data().host; - let end = dst.checked_add(cap).ok_or(HostError::PointerOutOfBounds)?; // Bounds-checked over the guest's whole declared region, so a buffer running // past memory is a wrong pointer rather than a truncated prefix being served… - let out = mem + let buf = mem .data_mut(&mut *caller) - .get_mut(dst..end) + .get_mut(range) .ok_or(HostError::PointerOutOfBounds)?; // …of which only the field cap is writable, so no call can exceed it whatever // the guest declared. - let out = &mut out[..cap.min(MAX_FIELD_BYTES)]; + let buf = &mut buf[..cap.min(MAX_FIELD_BYTES)]; - let n = fill(host, out)?; + let n = fill(host, buf)?; if n > MAX_FIELD_BYTES { return Err(HostError::DataFieldTooLarge); @@ -140,9 +127,9 @@ pub(crate) fn write_into( /// passed. /// /// `call` gets the guest's whole memory, so it can borrow any number of input -/// regions with [`region`] — which a `&mut` view of that memory would forbid. That -/// is why the answer goes through a buffer instead of straight into the guest as -/// [`write_into`]'s does. +/// regions with [`Region::read`] — which a `&mut` view of that memory would forbid. +/// That is why the answer goes through a buffer instead of straight into the guest +/// as [`write_into`]'s does. /// /// **The host is never told the guest's capacity**: it is offered the whole buffer /// and reports the value's true length, so the fit is decided here, with nothing yet @@ -151,10 +138,9 @@ pub(crate) fn write_into( /// The output is judged after the inputs, so a call with both bad reports the /// input's verdict. `NoMemExported` precedes both: there is no memory to validate a /// region against. -pub(crate) fn scratch_write( +pub(crate) fn write_buffered( caller: &mut Caller<'_, VmState<'_>>, - dst: i32, - cap: i32, + out: Region, call: impl FnOnce(&dyn HostFunctions, &[u8], &mut [u8]) -> HostResult, ) -> HostResult { let mem = memory(caller)?; @@ -166,21 +152,19 @@ pub(crate) fn scratch_write( let n = call(host, data, &mut state.out_buffer[..])?; - let (Ok(dst), Ok(cap)) = (usize::try_from(dst), usize::try_from(cap)) else { - return Err(HostError::InvalidParams); - }; + // `out` is checked here rather than before the call: the inputs are judged + // first, so a call with both malformed reports the input's verdict. + let range = out.range()?; + let cap = range.len(); if n > MAX_FIELD_BYTES { return Err(HostError::DataFieldTooLarge); } - let end = dst.checked_add(cap).ok_or(HostError::PointerOutOfBounds)?; - let out = data - .get_mut(dst..end) - .ok_or(HostError::PointerOutOfBounds)?; + let buf = data.get_mut(range).ok_or(HostError::PointerOutOfBounds)?; if n > cap { return Err(HostError::BufferTooSmall); } charge_transfer(state, n)?; - out[..n].copy_from_slice(&state.out_buffer[..n]); + buf[..n].copy_from_slice(&state.out_buffer[..n]); #[expect( clippy::cast_possible_truncation, clippy::cast_possible_wrap, @@ -190,10 +174,6 @@ pub(crate) fn scratch_write( Ok(n) } -// A `Caller` exists only during a host call, so everything above that takes one is -// unreachable from here; `tests/` covers those by running real modules against a -// fake host. - #[cfg(test)] mod tests { use super::*; diff --git a/crates/xrpl-wasm-vm/src/lib.rs b/crates/xrpl-wasm-vm/src/lib.rs index f6934705b3..933ee58628 100644 --- a/crates/xrpl-wasm-vm/src/lib.rs +++ b/crates/xrpl-wasm-vm/src/lib.rs @@ -16,6 +16,7 @@ )] mod abi; +mod region; mod register; mod vm; diff --git a/crates/xrpl-wasm-vm/src/region.rs b/crates/xrpl-wasm-vm/src/region.rs new file mode 100644 index 0000000000..06268396c8 --- /dev/null +++ b/crates/xrpl-wasm-vm/src/region.rs @@ -0,0 +1,50 @@ +use crate::vm::MAX_FIELD_BYTES; +use core::ops::Range; +use xrpl_host_functions::{HostError, HostResult}; + +/// A byte region as the guest declared it: the `(ptr, len)` pair off the wire, not +/// yet checked. +/// +/// Every byte parameter in this ABI is such a pair, so pairing them once at the wire +/// boundary is what keeps the helpers in `abi.rs` from each taking two loose integers +/// they could be handed in either order. +/// +/// It lives in a module of its own so that the fields are out of reach and +/// [`range`](Region::range) is the *only* way to indices — the check cannot be +/// skipped, only deferred. Construction is infallible for that reason: a call whose +/// output region is malformed is then refused in the order its own helper chooses, +/// rather than at the moment the pair happened to be formed. +#[derive(Copy, Clone)] +pub(crate) struct Region { + ptr: i32, + len: i32, +} + +impl Region { + pub(crate) fn new(ptr: i32, len: i32) -> Region { + Region { ptr, len } + } + + /// `start..end` as indices. The conversion is the negativity check — it fails on + /// exactly the negative values — and the addition guards a 32-bit `usize`, where + /// two `i32`s can sum past the end. + pub(crate) fn range(self) -> HostResult> { + let (Ok(start), Ok(len)) = (usize::try_from(self.ptr), usize::try_from(self.len)) else { + return Err(HostError::InvalidParams); + }; + let end = start + .checked_add(len) + .ok_or(HostError::PointerOutOfBounds)?; + Ok(start..end) + } + + /// The region's bytes, refused past the field cap. No copy: the slice aliases + /// `data`. + pub(crate) fn read(self, data: &[u8]) -> HostResult<&[u8]> { + let range = self.range()?; + if range.len() > MAX_FIELD_BYTES { + return Err(HostError::DataFieldTooLarge); + } + data.get(range).ok_or(HostError::PointerOutOfBounds) + } +} diff --git a/crates/xrpl-wasm-vm/src/register.rs b/crates/xrpl-wasm-vm/src/register.rs index a56906b037..9874c33816 100644 --- a/crates/xrpl-wasm-vm/src/register.rs +++ b/crates/xrpl-wasm-vm/src/register.rs @@ -1,4 +1,5 @@ -use crate::abi::{charged, read_borrowed, region, scratch_write, write_into}; +use crate::abi::{charged, read_borrowed, write_buffered, write_into}; +use crate::region::Region; use crate::vm::VmState; use wasmi::{Caller, Linker}; use xrpl_host_functions::{HostError, HostFunctionSpec}; @@ -29,7 +30,8 @@ pub(crate) fn register_host_functions( out_len: i32| -> Result { charged(&mut caller, HostFunctionSpec::GetLedgerSqn, |c| { - write_into(c, out_ptr, out_len, |host, out| host.get_ledger_sqn(out)) + let out = Region::new(out_ptr, out_len); + write_into(c, out, |host, out| host.get_ledger_sqn(out)) }) }, ), @@ -45,7 +47,8 @@ pub(crate) fn register_host_functions( &mut caller, HostFunctionSpec::GetCurrentLedgerObjField, |c| { - write_into(c, out_ptr, out_len, |host, out| { + let out = Region::new(out_ptr, out_len); + write_into(c, out, |host, out| { host.get_current_ledger_obj_field(field, out) }) }, @@ -62,9 +65,10 @@ pub(crate) fn register_host_functions( out_len: i32| -> Result { charged(&mut caller, HostFunctionSpec::Sha512Half, |c| { - scratch_write(c, out_ptr, out_len, |host, data, out| { - let input = region(data, data_ptr, data_len)?; - host.sha512_half(input, out) + let out = Region::new(out_ptr, out_len); + let input = Region::new(data_ptr, data_len); + write_buffered(c, out, |host, data, buf| { + host.sha512_half(input.read(data)?, buf) }) }) }, @@ -81,8 +85,8 @@ pub(crate) fn register_host_functions( -> Result { charged(&mut caller, HostFunctionSpec::Trace, |c| { let host = c.data().host; - let msg = read_borrowed(c, msg_ptr, msg_len)?; - let data = read_borrowed(c, data_ptr, data_len)?; + let msg = read_borrowed(c, Region::new(msg_ptr, msg_len))?; + let data = read_borrowed(c, Region::new(data_ptr, data_len))?; let msg = core::str::from_utf8(msg).map_err(|_| HostError::Decoding)?; host.trace(msg, data, as_hex != 0)?; Ok(0) @@ -99,7 +103,7 @@ pub(crate) fn register_host_functions( -> Result { charged(&mut caller, HostFunctionSpec::TraceNum, |c| { let host = c.data().host; - let msg = read_borrowed(c, msg_ptr, msg_len)?; + let msg = read_borrowed(c, Region::new(msg_ptr, msg_len))?; let msg = core::str::from_utf8(msg).map_err(|_| HostError::Decoding)?; host.trace_num(msg, number)?; Ok(0) diff --git a/crates/xrpl-wasm-vm/src/vm.rs b/crates/xrpl-wasm-vm/src/vm.rs index c311c98a47..d50f5b7bd7 100644 --- a/crates/xrpl-wasm-vm/src/vm.rs +++ b/crates/xrpl-wasm-vm/src/vm.rs @@ -55,7 +55,7 @@ pub(crate) struct VmState<'h> { /// have to resolve per instance: a cached handle would serve a call against the /// wrong instance's memory, which is a wrong answer rather than an error. pub(crate) memory: Option, - /// Where a host writes a value before [`crate::abi::scratch_write`] copies it + /// Where a host writes a value before [`crate::abi::write_buffered`] copies it /// to the guest. One buffer per run, so no call zero-fills one of its own. /// /// Inline rather than boxed: the store's data is built once and then only diff --git a/crates/xrpl-wasm-vm/tests/memory_policy.rs b/crates/xrpl-wasm-vm/tests/memory_policy.rs index 28f265bb78..3042165fdc 100644 --- a/crates/xrpl-wasm-vm/tests/memory_policy.rs +++ b/crates/xrpl-wasm-vm/tests/memory_policy.rs @@ -294,7 +294,7 @@ fn both_of_traces_regions_are_checked() { } // --------------------------------------------------------------------------- -// Both at once (`scratch_write`, via `sha512_half`) +// Both at once (`write_buffered`, via `sha512_half`) // --------------------------------------------------------------------------- /// A call with an input and an output region decides everything about the input @@ -362,11 +362,11 @@ fn a_read_write_output_obeys_the_write_rules() { /// A refused value reaches guest memory in no part, however much of it the host /// wrote. The host answers with 32 bytes it did write and a length it did not, so -/// the refusal happens with the value sitting in the run's scratch — and the +/// the refusal happens with the value sitting in the run's output buffer — and the /// guest's buffer has to come back untouched. /// /// Stronger than the contract asks for: a guest must not read its buffer on a -/// negative status. It holds because the scratch is copied to the guest only after +/// negative status. It holds because the buffer is copied to the guest only after /// the length, the bounds, the fit and the budget have all passed, so there is no /// window in which a refused value is in guest memory. #[test] diff --git a/docs/claude/redesign_impl.md b/docs/claude/redesign_impl.md index 2407f2fcbd..30b8e9ea0d 100644 --- a/docs/claude/redesign_impl.md +++ b/docs/claude/redesign_impl.md @@ -351,7 +351,7 @@ recovered `HostFuncWrapper.h` protos: | shape | count | helper | | --- | --- | --- | -| ≥1 byte input **and** a byte output | **38** | `scratch_write` | +| ≥1 byte input **and** a byte output | **38** | `write_buffered` | | byte output only | 9 | `write_into`, unchanged | | byte inputs only, or scalars | 18 | `read_borrowed` / `region`, unchanged | @@ -682,7 +682,7 @@ same one-instance-per-run assumption C10's cache would take on. the *output*" for the census that decided it and for why `MaybeUninit` is not the answer. - `read_write` is gone, replaced by `scratch_write`, and the input primitive split in + `read_write` is gone, replaced by `write_buffered`, and the input primitive split in two: `region(data, ptr, len)` does the validation and slicing against a plain `&[u8]`, and `read_borrowed` is now that over the guest's memory for the calls that read without writing. Taking bytes rather than a `Caller` is the whole trick — input @@ -724,7 +724,7 @@ same one-instance-per-run assumption C10's cache would take on. then only borrowed, so a kilobyte in it costs one move where a `Box` costs an allocation. **Lazy init is deferred to a benchmark, not rejected.** `Option<[u8; N]>` with `get_or_insert_with` is the shape (not `OnceCell`, which is for init behind a - shared borrow; `scratch_write` holds `&mut VmState`), and the case against it today is + shared borrow; `write_buffered` holds `&mut VmState`), and the case against it today is a magnitude argument that a measurement could overturn: it defers one ~1 KiB fill per run — invisible beside the `Module::new` that starts every run — and pays for it with a discriminant test on every host call, which is the direction C11 was moving cost away @@ -1038,6 +1038,48 @@ instead of error text to parse. D16's `gas = 0` decision belongs there too, sinc a TER choice. Deferred as before: macro-emitted `link_*` shims, the generated C header, the probe-module test. +## `Region`: the wire's `(ptr, len)` as one type (2026-08-03) + +Every byte parameter in the ABI is a `(ptr, len)` pair, so `crates/xrpl-wasm-vm/src/region.rs` +makes the pair a type. `abi.rs`'s helpers take one `Region` where they took two loose +`i32`s, and `register.rs` forms one per wire pair, next to the wasm parameter list where a +reader can check it against the signature. + +**What it does and does not check** is the part worth recording, because the obvious +expectation is wrong. It cannot catch a swapped pair: `Region::new(len, ptr)` compiles, and +no type can do better at that boundary — the values arrive as indistinguishable `i32`s in +positional order, so establishing the mapping is a job for a human reading it or for the +deferred shim generator emitting it. What it *does* enforce is that the pair cannot be used +unchecked: `range()` is the only way from a `Region` to indices, and it is where +`InvalidParams` (the conversion is the negativity check) and the end-overflow guard live. +Three copies of that conversion in `abi.rs` became one. + +**The type is in a module of its own, and that is load-bearing.** Rust privacy is +module-level, so with `Region` declared in `abi.rs` the helpers there could still read +`out.ptr` and skip `range()` — the invariant would have held by convention only. Separated, +an attempted bypass is `error[E0616]: field ptr of struct Region is private`, which is how +this was verified. + +**Construction is infallible on purpose.** Validating in `new` would hoist the output +region's verdict above the host call, and `write_buffered` deliberately judges the inputs +first (`a_read_write_checks_its_input_before_its_output` pins it, including the negative-`dst` +case). Deferring the check to `range()` is what lets the type exist without moving that +order. + +Two orderings did shift, both unobservable: `range()` runs its end-overflow guard before +the field-cap check, where `region()` had the cap first, and `write_into` can now answer +`PointerOutOfBounds` before `NoMemExported`. Both need `ptr + len` to overflow `usize`, +which two `i32`s cannot do on a 64-bit target — the guard is there for a 32-bit one. + +`Ptr`/`Len` as separate newtypes were considered and dropped. They catch only ptr↔len +confusion, not the mispairing that scales with the ABI, and they cannot reach the wire +either: `wasmi::WasmTy` looks implementable — public, no sealing supertrait — but its bound +names `UntypedVal`, which wasmi re-exports only through a **private** `mod core` +(`lib.rs:109-137`), so the impls cannot be written. Probed: `error[E0603]: module core is +private`. The escape hatch is a direct `wasmi_core` dependency pinned in lockstep with +wasmi's own, plus a `#[doc(hidden)]` method — not worth it on a consensus path, so host +function parameters stay `i32` and are paired on the first line of each arm. + ## The comment cut-back (done, 2026-08-03) **Done over `src/` and `tests/`, after C11 and with C12 deferred to a benchmark** — so no @@ -1063,8 +1105,10 @@ and `kWasmTransferLimit`, which are where the numbers are defined for the rest o system and are named in `the_limits_are_the_protocol_limits` for that reason. The parity evidence itself is not lost — it is in this document, and this is where it belongs. -The `scratch` field is now `VmState::out_buffer`. `abi::scratch_write` keeps its name; -if that reads inconsistently, the rename is a one-liner. +The buffer is `VmState::out_buffer` and the helper that stages through it is +`abi::write_buffered`, beside `abi::write_into` — the two ways a call's byte answer reaches +the guest, verb first in both. "Scratch" survives in this document as the name of the +design, not of anything in the code. Two TODOs were made honest rather than deleted, since both read as gaps and neither is one: the start-section TODO now says why wasmi 1.1 cannot close it and that the section is @@ -1099,7 +1143,7 @@ What it cut: notes explaining a borrow the compiler already enforces. - The same rationale on a field and on the function that reads it: `VmState::memory` keeps the arena-index invariant and `abi::memory` keeps only the two ways it is absent. -- Paragraphs duplicating this document — `scratch_write`'s case for its design went from +- Paragraphs duplicating this document — `write_buffered`'s case for its design went from five paragraphs to three short ones, the ABI-shape argument left here. - Every C++ citation, per the rule above.