oficina / docs Public

core: fix-round-2 — check content, not just liveness, in trap containment fuzz

Independent review verified all six fix-round-1 findings, then found the
containment fuzz's per-trial check only asserted a control document's
text_len was non-negative, never reading its content back. Silent
content corruption of another document would have passed unnoticed —
docs_core_text_read wasn't even bound in the test harness.

Bind docs_core_text_read (Harness::read_text), load each control's
expected plain text from its fixture JSON, and compare byte-for-byte
after every trial, not just check the document still answers text_len.

Proved the strengthened check by reintroducing the fix-round-1 borrow
leak (as the reviewer did) and confirming the fuzz still fails, at the
same trial 10 the reviewer found. That failure is via the pre-existing
liveness/wedge assertion, not the new content check, because a leaked
RefCell borrow on the handle map can't reach into an already-live Doc's
own content — recorded honestly rather than left unremarked. Separately
proved the content assertion itself fires correctly on real divergence
via a throwaway harness-side fault injection (not committed): with a
control's expected text deliberately corrupted, the assertion fails
immediately with the exact document/trial/expected-vs-actual bytes.

Also: borrow_leak_repro_unicode_c3a9_to_00a9 used an input that never
actually traps in this build, so it passed unchanged even with the bug
reintroduced — switched to a confirmed-trapping mutation and renamed to
match, with an explicit assert that it still traps. And corrected the
APPLY_FUEL_BUDGET comment's headroom claim (100x) to the measured
figure (~4,500x against the highest observed control cost).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011F5CivVm4CmL9ByfSSAaXA

Paul Campbell pushed by paul@keymail.dev 1 month ago e04e106492c12a10e832a8b8c39dc9fd5971054b parent db768c5

1 file changed, +167 −44

core/tests/wasm_trap_containment.rs +167 −44

Intent

full ledger: core-wire-compat

diff --git a/core/tests/wasm_trap_containment.rs b/core/tests/wasm_trap_containment.rs

index 151e9f4..3750fa3 100644

--- a/core/tests/wasm_trap_containment.rs

+++ b/core/tests/wasm_trap_containment.rs

@@ -56,6 +56,17 @@

//! containment property under test (no cross-document damage, instance

//! still usable) applies equally to both.

//!

+//! # Fix-round-2: liveness was checked, not content (finding A)

+//!

+//! Fix-round-1's per-trial check only asserted `text_len(control) >= 0` —

+//! that the control document still *exists* — and never read its content

+//! back at all, so silent content corruption of another document (one of

+//! the two failure modes this test exists to catch) would have passed

+//! silently. `docs_core_text_read` is now bound in the `Harness`

+//! (`read_text`), and every trial compares each control document's full

+//! text content byte-for-byte against the value it was seeded with, not

+//! just checking that a length comes back non-negative.

+//!

//! This test builds `core`'s wasm32-unknown-unknown release artifact

//! itself (via `cargo build`) so it always exercises current source, not a

//! possibly-stale artifact left over from a previous `scripts/build-wasm.sh`

@@ -66,10 +77,11 @@ use std::process::Command;

use wasmi::{Config, Engine, Instance, Linker, Memory, Module, Store, TypedFunc};

-/// Fuel budget for a single `docs_core_apply` call. Calibrated against this

-/// test's own control-document applies (real, valid fixtures), which

-/// consume on the order of a few hundred thousand fuel units each — this

-/// budget gives roughly 100x headroom above that, while stopping the

+/// Fuel budget for a single `docs_core_apply` call. Measured control-

+/// document applies (real, valid fixtures) consumed 21,978 / 21,989 /

+/// 44,281 / 31,382 fuel units (hello / unicode / formatted / concurrent —

+/// see this test's own `eprintln!` output). 200,000,000 is roughly 4,500x

+/// the highest of those (44,281, `formatted`), while stopping the

/// 163-second pathological input (see the module doc comment) after a

/// small fraction of a second instead.

const APPLY_FUEL_BUDGET: u64 = 200_000_000;

@@ -82,6 +94,7 @@ struct Harness {

doc_new: TypedFunc<(u32, u32, u64), u32>,

apply: TypedFunc<(u32, u32, u32), i32>,

text_len: TypedFunc<u32, i32>,

+ text_read: TypedFunc<(u32, u32, u32), i32>,

}

impl Harness {

@@ -122,6 +135,9 @@ impl Harness {

let text_len = instance

.get_typed_func::<u32, i32>(&store, "docs_core_text_len")

.expect("docs_core_text_len export");

+ let text_read = instance

+ .get_typed_func::<(u32, u32, u32), i32>(&store, "docs_core_text_read")

+ .expect("docs_core_text_read export");

Self {

store,

memory,

@@ -130,6 +146,7 @@ impl Harness {

doc_new,

apply,

text_len,

+ text_read,

}

@@ -173,7 +190,7 @@ impl Harness {

let after = self.store.get_fuel().unwrap_or(0);

let consumed = APPLY_FUEL_BUDGET.saturating_sub(after);

// Restore a large budget for bookkeeping calls (doc_new, text_len,

- // free) that aren't under test here.

+ // text_read, free) that aren't under test here.

self.store.set_fuel(u64::MAX / 2).ok();

let _ = before;

if let Err(pl) = self.free_buf(ptr, len) {

@@ -189,6 +206,43 @@ impl Harness {

fn text_len(&mut self, handle: u32) -> Result<i32, wasmi::Error> {

self.text_len.call(&mut self.store, handle)

}

+ /// Reads `handle`'s full text content back out of wasm memory,

+ /// byte-for-byte: calls `docs_core_text_len` to size a buffer,

+ /// allocates it, calls `docs_core_text_read` into it, copies the

+ /// written bytes out, and frees the buffer. Fix-round-2 finding A:

+ /// added so the containment test can compare *content*, not just

+ /// whether a handle still answers `text_len` non-negatively.

+ ///

+ /// Panics (rather than returning a `Result`) on ERR_NO_DOC or a

+ /// length/read-count mismatch — those indicate a caller bug (reading a

+ /// handle that was never confirmed live) or an ABI contract violation

+ /// distinct from the traps this harness expects and handles via `Err`.

+ fn read_text(&mut self, handle: u32) -> Result<Vec<u8>, wasmi::Error> {

+ let len = self.text_len.call(&mut self.store, handle)?;

+ assert!(

+ len >= 0,

+ "read_text called on handle {handle} which text_len rejects: rc={len} (ERR_NO_DOC)"

+ );

+ let len = len as u32;

+ if len == 0 {

+ return Ok(Vec::new());

+ }

+ let out_ptr = self.alloc.call(&mut self.store, len)?;

+ assert_ne!(out_ptr, 0, "docs_core_alloc failed for {len} bytes (text_read buffer)");

+ let rc = self.text_read.call(&mut self.store, (handle, out_ptr, len))?;

+ assert_eq!(

+ rc, len as i32,

+ "docs_core_text_read returned {rc} bytes written, expected {len} \

+ (handle {handle} changed length between text_len and text_read?)"

+ );

+ let mut buf = vec![0u8; len as usize];

+ self.memory

+ .read(&mut self.store, out_ptr as usize, &mut buf)

+ .expect("read within a just-written buffer cannot be out of bounds");

+ self.free.call(&mut self.store, (out_ptr, len))?;

+ Ok(buf)

+ }

}

/// Builds `core`'s wasm32-unknown-unknown release artifact from current

@@ -217,11 +271,15 @@ fn build_wasm() -> Vec<u8> {

std::fs::read(&artifact).unwrap_or_else(|e| panic!("reading {artifact:?}: {e}"))

}

-fn fixtures() -> Vec<(&'static str, Vec<u8>)> {

- let dir = PathBuf::from(env!("CARGO_MANIFEST_DIR"))

+fn fixtures_dir() -> PathBuf {

+ PathBuf::from(env!("CARGO_MANIFEST_DIR"))

.parent()

.unwrap()

- .join("fixtures/data");

+ .join("fixtures/data")

+fn fixtures() -> Vec<(&'static str, Vec<u8>)> {

+ let dir = fixtures_dir();

["hello", "unicode", "formatted", "concurrent"]

.iter()

.map(|name| {

@@ -231,6 +289,21 @@ fn fixtures() -> Vec<(&'static str, Vec<u8>)> {

.collect()

}

+/// The plain-text `text` field from a fixture's `{name}.expected.json` —

+/// the expected content of the root text type once `{name}.update.bin` is

+/// applied to a fresh document. Used to check control documents'

+/// *content*, not just that they still exist (fix-round-2 finding A).

+fn expected_text(name: &str) -> String {

+ let path = fixtures_dir().join(format!("{name}.expected.json"));

+ let raw = std::fs::read_to_string(&path).unwrap_or_else(|e| panic!("reading {path:?}: {e}"));

+ let json: serde_json::Value =

+ serde_json::from_str(&raw).unwrap_or_else(|e| panic!("parsing {path:?}: {e}"));

+ json["text"]

+ .as_str()

+ .unwrap_or_else(|| panic!("{path:?} has no string \"text\" field"))

+ .to_string()

// Deterministic xorshift64 PRNG so a failure is reproducible without a

// crates.io dependency (fix-round-1 finding 5: "use a fixed seed").

struct Rng(u64);

@@ -263,8 +336,10 @@ fn mutate(rng: &mut Rng, base: &[u8]) -> Vec<u8> {

/// The actual regression test: a wasm trap (or fuel exhaustion — see the

/// module doc comment on the hang finding) on one document's `apply` must

-/// not affect any other document in the same instance, and must not stop

-/// the instance from creating new documents afterward.

+/// not affect any other document in the same instance — neither its

+/// existence (`text_len` still answers) nor its *content* (`text_read`

+/// still returns exactly what it was seeded with, fix-round-2 finding A)

+/// — and must not stop the instance from creating new documents afterward.

///

/// Mutation-fuzzes the real fixtures (never edits them) with a fixed seed

/// (`0xC0FFEE`) over a bounded 1500 trials — enough to reliably reproduce

@@ -274,8 +349,8 @@ fn mutate(rng: &mut Rng, base: &[u8]) -> Vec<u8> {

/// wire bytes and not something this crate can fix. What this test asserts

/// is the containment property this crate *can* guarantee: every trial's

/// outcome — success, a clean negative error code, or a contained failure —

-/// never damages any other document, and `docs_core_doc_new` keeps working

-/// afterward.

+/// never damages any other document's existence or content, and

+/// `docs_core_doc_new` keeps working afterward.

#[test]

fn trap_on_one_document_does_not_corrupt_others_or_wedge_the_instance() {

let wasm = build_wasm();

@@ -283,7 +358,9 @@ fn trap_on_one_document_does_not_corrupt_others_or_wedge_the_instance() {

let fixtures = fixtures();

// Three "control" documents, representing other live documents in the

- // same instance, each seeded with real, valid content up front.

+ // same instance, each seeded with real, valid content up front. Each

+ // carries its *expected plain-text content*, not just its raw update

+ // bytes, so every trial can check content, not just liveness.

let mut controls = Vec::new();

for (i, (name, bytes)) in fixtures.iter().enumerate() {

let handle = h

@@ -294,7 +371,14 @@ fn trap_on_one_document_does_not_corrupt_others_or_wedge_the_instance() {

let rc = result.expect("applying a known-good fixture must not trap");

assert_eq!(rc, 0, "control document {name} failed to apply its own fixture");

eprintln!("control {name}: fuel consumed = {fuel}");

- controls.push((name, handle, bytes.clone()));

+ let expected = expected_text(name);

+ let got = h.read_text(handle).expect("reading a freshly-seeded control must not trap");

+ assert_eq!(

+ got,

+ expected.as_bytes(),

+ "control document {name} content wrong immediately after seeding"

+ );

+ controls.push((name, handle, expected));

}

let mut rng = Rng(0xC0FFEE_u64);

@@ -341,8 +425,10 @@ fn trap_on_one_document_does_not_corrupt_others_or_wedge_the_instance() {

}

// The containment check: whatever just happened to `handle`, every

- // control document must still be fully intact.

- for (cname, chandle, cbytes) in &controls {

+ // control document must still exist AND still hold exactly the

+ // content it was seeded with (fix-round-2 finding A — a length

+ // check alone would miss silent content corruption).

+ for (cname, chandle, cexpected) in &controls {

let len = h.text_len(*chandle).unwrap_or_else(|e| {

panic!(

"control document {cname} became unreachable after trial {i} \

@@ -355,7 +441,21 @@ fn trap_on_one_document_does_not_corrupt_others_or_wedge_the_instance() {

"control document {cname} lost after trial {i} (fixture {name}): \

text_len returned {len} (ERR_NO_DOC)"

);

- let _ = cbytes; // identity already proven at seed time; length staying >=0 is the liveness check here

+ let got = h.read_text(*chandle).unwrap_or_else(|e| {

+ panic!(

+ "control document {cname} text_read trapped after trial {i} \

+ (fixture {name}, {} mutated bytes): {e}",

+ mutated.len()

+ )

+ });

+ assert_eq!(

+ got,

+ cexpected.as_bytes(),

+ "control document {cname} CONTENT CORRUPTED after trial {i} \

+ (fixture {name}, {} mutated bytes): expected {cexpected:?}, got {:?}",

+ mutated.len(),

+ String::from_utf8_lossy(&got),

+ );

}

@@ -389,55 +489,78 @@ fn trap_on_one_document_does_not_corrupt_others_or_wedge_the_instance() {

assert_eq!(rc, 0);

}

-/// Focused regression test for the exact scenario the fix-round-1 review

-/// reported: the `unicode` fixture with byte 16 flipped `c3` -> `00`

-/// (corrupting the `é` in "héllo" to a bare NUL followed by a dangling

-/// UTF-8 continuation byte). Kept separate from the broader fuzz above so

+/// Focused, named regression test for fix-round-1 finding 1 (the leaked

+/// `RefCell` borrow), using a confirmed-trapping mutation of the `unicode`

+/// fixture, found by the fuzz test above:

+/// `01010100040107636f6e74656e741168c3a96c6c6f20f09f8c8d206361ffc3a900`

+/// (3 bytes flipped from the real fixture), which reliably yields

+/// `TrapCode(MemoryOutOfBounds)`. Kept separate from the broader fuzz so

/// this specific, named repro can't silently stop being exercised if the

/// fuzz's RNG sequence or trial count ever changes.

///

-/// Empirically (see the fix-round-1 report) this exact single-byte flip

-/// does not itself trap through `docs_core_apply` — the invalid bytes are

+/// Fix-round-2 finding B: an earlier version of this test used the

+/// coordinator's originally-named byte flip instead — `unicode`, byte 16,

+/// `c3` -> `00` (corrupting the `é` in "héllo"). That flip does **not**

+/// itself trap through `docs_core_apply` in this build (verified in both

+/// fix-round-1's native and wasm32 testing: the invalid bytes are simply

/// accepted by `yrs`'s unchecked UTF-8 decode and applied without

-/// complaint, only becoming a hazard if something downstream inspects the

-/// string char-by-char (a Rust `Debug`-formats it, for instance). It is

-/// included here anyway, under the same harness as every other trial in

-/// this file, as the coordinator-specified named repro: whatever the

-/// current build does with it, the containment property must hold.

+/// complaint), so a test built on it passed unchanged even with the

+/// fix-round-1 bug reintroduced — it guarded nothing, despite its name.

+/// This version uses the mutation above instead, which does genuinely

+/// trap, so reintroducing the bug actually fails this test (verified — see

+/// the fix-round-2 report).

#[test]

-fn borrow_leak_repro_unicode_c3a9_to_00a9() {

+fn borrow_leak_repro_trapping_unicode_mutation() {

let wasm = build_wasm();

let mut h = Harness::new(&wasm);

let fixtures = fixtures();

- let (_, unicode_bytes) = fixtures.iter().find(|(n, _)| *n == "unicode").unwrap();

+ let hello = &fixtures.iter().find(|(n, _)| *n == "hello").unwrap().1;

+ let hello_expected = expected_text("hello");

- let mut corrupted = unicode_bytes.clone();

- assert_eq!(

- (corrupted[16], corrupted[17]),

- (0xc3, 0xa9),

- "fixture layout changed; byte 16..18 is no longer the targeted c3 a9"

- );

- corrupted[16] = 0x00;

+ let trapping: Vec<u8> = {

+ let hex = "01010100040107636f6e74656e741168c3a96c6c6f20f09f8c8d206361ffc3a900";

+ (0..hex.len())

+ .step_by(2)

+ .map(|i| u8::from_str_radix(&hex[i..i + 2], 16).unwrap())

+ .collect()

+ };

// A live, unrelated control document, created *before* the corrupted

- // apply, standing in for "other documents in the instance."

+ // apply, standing in for "other documents in the instance" — checked

+ // for content, not just existence.

let control = h

.doc_new("control-before-corruption", 42)

.expect("doc_new must not trap");

- let (control_result, _) = h.apply(control, &fixtures.iter().find(|(n, _)| *n == "hello").unwrap().1);

+ let (control_result, _) = h.apply(control, hello);

assert_eq!(control_result.unwrap(), 0);

+ let control_content = h.read_text(control).expect("reading the freshly-seeded control");

+ assert_eq!(control_content, hello_expected.as_bytes());

let handle = h.doc_new("corrupted-unicode", 7).expect("doc_new must not trap");

assert_ne!(handle, 0);

- let (result, _fuel) = h.apply(handle, &corrupted);

- eprintln!("borrow_leak_repro: apply(corrupted c3->00) = {result:?}");

+ let (result, _fuel) = h.apply(handle, &trapping);

+ eprintln!("borrow_leak_repro: apply(trapping mutation) = {result:?}");

+ assert!(

+ result.is_err(),

+ "this mutation is expected to trap (TrapCode::MemoryOutOfBounds); got {result:?} instead \

+ — if this ever starts succeeding, the mutation stopped being a valid repro and needs replacing"

+ );

- // Whatever just happened (trap or clean result), the pre-existing

- // control document must be unaffected...

+ // Whatever just happened, the pre-existing control document must be

+ // unaffected — both still there AND still holding its original

+ // content, not just answering text_len non-negatively.

let control_len = h

.text_len(control)

.expect("control document must survive the corrupted apply");

assert!(control_len >= 0);

+ let control_content_after = h

+ .read_text(control)

+ .expect("control document must still be readable after the corrupted apply");

+ assert_eq!(

+ control_content_after,

+ hello_expected.as_bytes(),

+ "control document content changed after the trapping apply to a different document"

+ );

// ...and doc_new must still work afterward — the exact property

// fix-round-1 finding 1 was about (a leaked RefCell borrow would make

@@ -446,7 +569,7 @@ fn borrow_leak_repro_unicode_c3a9_to_00a9() {

.doc_new("after-corruption", 99)

.expect("doc_new must keep working after the corrupted apply");

assert_ne!(after, 0, "doc_new returned a null handle after the corrupted apply");

- let (after_result, _) = h.apply(after, &fixtures.iter().find(|(n, _)| *n == "hello").unwrap().1);

+ let (after_result, _) = h.apply(after, hello);

assert_eq!(

after_result.expect("a brand-new document must still apply a good update"),