From 66a08fe3f3dd15f4af5e8d7edffca982d60068d6 Mon Sep 17 00:00:00 2001 From: Ralf Anton Beier Date: Fri, 10 Jul 2026 13:04:19 +0200 Subject: [PATCH] fix(fuse): gate the unsound shared+rebase auto path (#326) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--memory auto` silently selected shared memory + address rebasing for grow-free multi-memory inputs. That is UNSOUND (#326): `convert_memarg` only rebases the static memarg offset (and bulk-memory ops); the dynamic address operand of ordinary i32.load/i32.store is never rebased. Real components address heap/shadow-stack via computed pointers, so shared fusion silently overlaps their private memory — wrong results, no trap, no error. Confirmed in `rewriter.rs` (convert_memarg; needs_rebase_locals gated to Memory{Copy,Fill, Init}) and reproduced end-to-end. Gate now (LS-D-1 — emit correct output or none, never plausible-but-wrong): - `resolve_auto_memory_strategy` no longer selects shared+rebase; Auto always resolves to multi-memory (sound). A `log::warn!` explains the gate. - Explicit `--memory shared --address-rebase` remains an opt-in but now warns loudly that it is unsound for computed-pointer access. - Corrected the docstrings that claimed the shared path was "statically sound". Tests: - `auto_selects_shared_for_growfree_inputs` → `auto_gates_shared_for_growfree_ inputs_326` (asserts multi + 2 separate memories = the correctness #326 protects); `ls_m_7_auto_reprobes_after_add_component` re-probes via memory count (2 → 3) since both resolutions are now multi. - `test_304_identity_direct_adapter_is_inlined` now requests `SharedMemory` explicitly — the #304 inlining feature runs on the shared path, which Auto no longer selects; adapter inlining is orthogonal to the memory-access unsoundness #326 gates. Interim safety gate; correct runtime dynamic-address rebasing in rewriter.rs (Tier-5) is the follow-up "fix properly" step. Safety docs (ADR-4, loss-scenarios, safety-requirements) still describe the old auto→shared default and should be reconciled in a traceability follow-up. Refs #326. Co-Authored-By: Claude Opus 4.8 (1M context) --- meld-core/src/lib.rs | 52 +++++++++--- meld-core/tests/auto_memory.rs | 105 +++++++++++------------- meld-core/tests/cross_component_call.rs | 8 ++ 3 files changed, 97 insertions(+), 68 deletions(-) diff --git a/meld-core/src/lib.rs b/meld-core/src/lib.rs index b41f67c..6bc12b8 100644 --- a/meld-core/src/lib.rs +++ b/meld-core/src/lib.rs @@ -445,16 +445,35 @@ impl Fuser { } return result; } + // #326: explicit `--memory shared --address-rebase` is an opt-in to an + // unsound transform — address rebasing does not rebase computed memory + // addresses, so it silently corrupts real components. `auto` no longer + // selects this path; when a caller requests it explicitly, warn loudly + // rather than corrupt silently (LS-D-1). The build still proceeds. + if self.config.memory_strategy == MemoryStrategy::SharedMemory + && self.config.address_rebasing + { + log::warn!( + "memory strategy: explicit shared-memory + address rebasing is UNSOUND \ + for components that access memory via computed pointers (#326) — the \ + dynamic address operand of ordinary loads/stores is not rebased, so \ + per-component memory can silently collide. Prefer `--memory multi` \ + unless every input addresses memory only via static offsets." + ); + } self.fuse_with_stats_resolved() } /// Resolve `MemoryStrategy::Auto` against the added components. /// - /// Shared memory + address rebasing is selected only when it is - /// statically sound AND buys anything: no input module can grow its - /// memory (`memory_probe`, merger Bug #7) and the inputs carry at - /// least two memories (with fewer, multi-memory output is already - /// single-memory). Any user-supplied `address_rebasing` value is + /// Auto always selects **multi-memory** — it is the sound strategy. Shared + /// memory + address rebasing was previously auto-selected for grow-free, + /// multi-memory inputs, but that path is **unsound** (#326): rebasing does + /// not relocate the dynamic address operand of ordinary loads/stores, so + /// components addressing memory via computed pointers silently collide. + /// Until correct dynamic rebasing lands, Auto never picks shared+rebase; + /// it remains reachable only via explicit `--memory shared --address-rebase` + /// (which warns loudly). Any user-supplied `address_rebasing` value is /// overridden — Auto owns both knobs. fn resolve_auto_memory_strategy(&mut self) { let mut memory_count = 0usize; @@ -488,12 +507,25 @@ impl Fuser { self.config.memory_strategy = MemoryStrategy::MultiMemory; self.config.address_rebasing = false; } else { - log::info!( - "memory strategy auto: {memory_count} memories, no memory.grow; \ - selecting shared memory with address rebasing" + // #326: address rebasing does NOT rebase the dynamic address + // operand of ordinary loads/stores (only the static memarg offset + // and bulk-memory ops) — so shared-memory fusion silently corrupts + // any component that addresses memory via a computed pointer (heap, + // shadow stack — i.e. all real components). Until correct dynamic + // rebasing lands, `auto` must NOT silently pick shared+rebase. + // Fall back to multi-memory, which is sound (LS-D-1: emit correct + // output, never a plausible-but-wrong one). Explicit + // `--memory shared --address-rebase` remains available as an + // opt-in, and warns loudly (see `warn_if_unsound_rebasing`). + log::warn!( + "memory strategy auto: {memory_count} memories, no memory.grow — \ + shared-memory fusion would apply here, but address rebasing is \ + unsound for computed memory addresses (#326); selecting \ + multi-memory instead. Use `--memory shared --address-rebase` to \ + override explicitly (unsound; corrupts computed-pointer access)." ); - self.config.memory_strategy = MemoryStrategy::SharedMemory; - self.config.address_rebasing = true; + self.config.memory_strategy = MemoryStrategy::MultiMemory; + self.config.address_rebasing = false; } } diff --git a/meld-core/tests/auto_memory.rs b/meld-core/tests/auto_memory.rs index 6bd7d35..b4677e7 100644 --- a/meld-core/tests/auto_memory.rs +++ b/meld-core/tests/auto_memory.rs @@ -1,11 +1,13 @@ -//! `MemoryStrategy::Auto` resolution — integration oracle for #172. +//! `MemoryStrategy::Auto` resolution — integration oracle for #172 / #326. //! -//! The default memory strategy is `Auto`: shared memory + address rebasing -//! when no input module contains `memory.grow` and there are at least two -//! memories to merge, multi-memory otherwise. The point of the feature is -//! that out-of-the-box `meld fuse` output flows through `wasm-opt → synth` -//! with no extra flags, WITHOUT ever selecting the shared form where it is -//! unsound (merger Bug #7: shared memory breaks under `memory.grow`). +//! The default memory strategy is `Auto`. It resolves to **multi-memory**: +//! shared memory + address rebasing was previously auto-selected for grow-free +//! multi-memory inputs, but that path is UNSOUND (#326 — rebasing does not +//! relocate the dynamic address operand of ordinary loads/stores, so +//! computed-pointer access silently collides across components), so Auto no +//! longer selects it. Shared+rebase remains reachable only via explicit +//! `--memory shared --address-rebase`, which warns loudly. (Historically Auto +//! also always avoided shared under `memory.grow` — merger Bug #7.) //! //! The oracle here mirrors the issue's repro: `wasm-opt` (and any //! standards-default validator) rejects multi-memory modules. So the fused @@ -19,9 +21,6 @@ use wasm_encoder::{ ExportKind, ExportSection, Function, FunctionSection, Instruction, MemorySection, MemoryType, Module, ModuleSection, TypeSection, }; -use wasmtime::{Engine, Instance, Module as RuntimeModule, Store}; - -const WASM_PAGE_SIZE: usize = 65_536; fn build_component(module: Module) -> Vec { let mut component = Component::new(); @@ -178,52 +177,33 @@ fn validates_without_multimemory(wasm: &[u8]) -> bool { .is_ok() } -/// Two grow-free components, one memory each → Auto must pick shared + -/// rebasing: single-memory output, accepted by a no-flags validator, and -/// behaviourally correct (B's data lands at its rebased base, A's intact). +/// #326: address rebasing does not relocate the dynamic address operand of +/// ordinary loads/stores, so shared-memory fusion silently corrupts any +/// component that addresses memory via a computed pointer. Auto therefore no +/// longer selects shared+rebase even for grow-free inputs — it falls back to +/// multi-memory, the sound strategy (LS-D-1: correct output, never a +/// plausible-but-wrong one). Shared remains reachable only via explicit +/// `--memory shared --address-rebase`, which warns loudly. #[test] -fn auto_selects_shared_for_growfree_inputs() { +fn auto_gates_shared_for_growfree_inputs_326() { let component_a = build_component(build_module_a()); let component_b = build_component(build_module_b(false)); let (fused, stats) = fuse_default(&[component_a, component_b]); - assert_eq!(stats.memory_strategy, "shared"); + assert_eq!( + stats.memory_strategy, "multi", + "auto must gate the unsound shared+rebase path (#326) and select multi" + ); assert_eq!( output_memory_count(&fused), - 1, - "auto-resolved shared fusion must produce a single memory" + 2, + "multi-memory keeps each component's memory separate — no silent \ + cross-component collision (the correctness #326 protects)" ); assert!( - validates_without_multimemory(&fused), - "fused output must validate without --enable-multimemory (#172)" - ); - - // Behaviour: the single memory holds A's fill at 0 and B's data one - // page in (B's rebased base). - let engine = Engine::default(); - let module = RuntimeModule::new(&engine, &fused).unwrap(); - let mut store = Store::new(&engine, ()); - let instance = Instance::new(&mut store, &module, &[]).unwrap(); - - instance - .get_typed_func::<(), ()>(&mut store, "a_fill") - .unwrap() - .call(&mut store, ()) - .unwrap(); - instance - .get_typed_func::<(), ()>(&mut store, "b_init") - .unwrap() - .call(&mut store, ()) - .unwrap(); - - let memory = instance.get_memory(&mut store, "memory").unwrap(); - let data = memory.data(&store); - assert_eq!(&data[0..4], &[0x11, 0x11, 0x11, 0x11], "A's region"); - assert_eq!( - &data[WASM_PAGE_SIZE..WASM_PAGE_SIZE + 4], - &[1, 2, 3, 4], - "B's region must sit at its rebased base, not clobber A" + !validates_without_multimemory(&fused), + "sanity: the multi-memory form is the one a no-flags validator rejects" ); } @@ -265,11 +245,12 @@ fn auto_keeps_multi_for_single_memory_input() { } /// Mythos finding A (PR #220): Auto resolution must be re-derived from the -/// CURRENT component set on every fuse. A fuse → `add_component` → fuse -/// sequence must not reuse the first fuse's stale "shared" resolution when -/// the newly added component grows memory — the second fuse must succeed -/// and resolve to multi. (`ls_m_7_` prefix: LS-M-7 / UCA-M-11 regression, -/// run by the LS-N verification gate.) +/// CURRENT component set on every fuse. Post-#326 Auto always resolves to +/// multi-memory (shared+rebase is gated off as unsound), so re-probing is +/// verified by the memory layout re-deriving: a grow-free pair fuses to 2 +/// memories, and after adding a third component the next fuse re-derives to +/// 3 — not a stale 2. (`ls_m_7_` prefix: LS-M-7 / UCA-M-11 regression, run by +/// the LS-N verification gate.) #[test] fn ls_m_7_auto_reprobes_after_add_component() { let component_a = build_component(build_module_a()); @@ -280,21 +261,29 @@ fn ls_m_7_auto_reprobes_after_add_component() { fuser.add_component_named(&component_a, Some("a")).unwrap(); fuser.add_component_named(&component_b, Some("b")).unwrap(); - let (_, stats) = fuser.fuse_with_stats().unwrap(); - assert_eq!(stats.memory_strategy, "shared", "grow-free pair → shared"); + let (fused1, stats) = fuser.fuse_with_stats().unwrap(); + assert_eq!( + stats.memory_strategy, "multi", + "grow-free pair → multi (#326 gates the unsound shared+rebase path)" + ); + assert_eq!( + output_memory_count(&fused1), + 2, + "grow-free pair fuses to 2 memories" + ); fuser .add_component_named(&component_grow, Some("grower")) .unwrap(); let (fused, stats) = fuser .fuse_with_stats() - .expect("second fuse must re-resolve, not reuse the stale shared plan"); + .expect("second fuse must re-resolve against the current component set"); + assert_eq!(stats.memory_strategy, "multi"); assert_eq!( - stats.memory_strategy, "multi", - "a growing component added after a previous fuse must flip the \ - resolution back to multi" + output_memory_count(&fused), + 3, + "the added component must be re-probed: 3 memories, not a stale 2" ); - assert_eq!(output_memory_count(&fused), 3); } /// Explicit strategies are untouched by Auto: `MultiMemory` still produces diff --git a/meld-core/tests/cross_component_call.rs b/meld-core/tests/cross_component_call.rs index f5e8746..6d5c665 100644 --- a/meld-core/tests/cross_component_call.rs +++ b/meld-core/tests/cross_component_call.rs @@ -168,9 +168,17 @@ fn test_304_identity_direct_adapter_is_inlined() { return; }; + // Identity-adapter inlining (#304) is exercised on the shared-memory + // fusion path. Request it explicitly: since #326, `auto` no longer selects + // shared+rebase (it is unsound for computed-pointer memory access), so this + // feature test must opt into shared itself. The unsoundness #326 gates is + // about memory *access*, not adapter inlining, so exercising shared here to + // check the inlining + validity is fine. let mut fuser = Fuser::new(FuserConfig { attestation: false, component_provenance: false, + memory_strategy: MemoryStrategy::SharedMemory, + address_rebasing: true, ..Default::default() }); fuser