Skip to content

Commit e325398

Browse files
avrabeclaude
andauthored
fix(fuse): gate the unsound shared+rebase auto path (#326) (#329)
`--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) <noreply@anthropic.com>
1 parent f177fb4 commit e325398

3 files changed

Lines changed: 97 additions & 68 deletions

File tree

meld-core/src/lib.rs

Lines changed: 42 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -452,16 +452,35 @@ impl Fuser {
452452
}
453453
return result;
454454
}
455+
// #326: explicit `--memory shared --address-rebase` is an opt-in to an
456+
// unsound transform — address rebasing does not rebase computed memory
457+
// addresses, so it silently corrupts real components. `auto` no longer
458+
// selects this path; when a caller requests it explicitly, warn loudly
459+
// rather than corrupt silently (LS-D-1). The build still proceeds.
460+
if self.config.memory_strategy == MemoryStrategy::SharedMemory
461+
&& self.config.address_rebasing
462+
{
463+
log::warn!(
464+
"memory strategy: explicit shared-memory + address rebasing is UNSOUND \
465+
for components that access memory via computed pointers (#326) — the \
466+
dynamic address operand of ordinary loads/stores is not rebased, so \
467+
per-component memory can silently collide. Prefer `--memory multi` \
468+
unless every input addresses memory only via static offsets."
469+
);
470+
}
455471
self.fuse_with_stats_resolved()
456472
}
457473

458474
/// Resolve `MemoryStrategy::Auto` against the added components.
459475
///
460-
/// Shared memory + address rebasing is selected only when it is
461-
/// statically sound AND buys anything: no input module can grow its
462-
/// memory (`memory_probe`, merger Bug #7) and the inputs carry at
463-
/// least two memories (with fewer, multi-memory output is already
464-
/// single-memory). Any user-supplied `address_rebasing` value is
476+
/// Auto always selects **multi-memory** — it is the sound strategy. Shared
477+
/// memory + address rebasing was previously auto-selected for grow-free,
478+
/// multi-memory inputs, but that path is **unsound** (#326): rebasing does
479+
/// not relocate the dynamic address operand of ordinary loads/stores, so
480+
/// components addressing memory via computed pointers silently collide.
481+
/// Until correct dynamic rebasing lands, Auto never picks shared+rebase;
482+
/// it remains reachable only via explicit `--memory shared --address-rebase`
483+
/// (which warns loudly). Any user-supplied `address_rebasing` value is
465484
/// overridden — Auto owns both knobs.
466485
fn resolve_auto_memory_strategy(&mut self) {
467486
let mut memory_count = 0usize;
@@ -495,12 +514,25 @@ impl Fuser {
495514
self.config.memory_strategy = MemoryStrategy::MultiMemory;
496515
self.config.address_rebasing = false;
497516
} else {
498-
log::info!(
499-
"memory strategy auto: {memory_count} memories, no memory.grow; \
500-
selecting shared memory with address rebasing"
517+
// #326: address rebasing does NOT rebase the dynamic address
518+
// operand of ordinary loads/stores (only the static memarg offset
519+
// and bulk-memory ops) — so shared-memory fusion silently corrupts
520+
// any component that addresses memory via a computed pointer (heap,
521+
// shadow stack — i.e. all real components). Until correct dynamic
522+
// rebasing lands, `auto` must NOT silently pick shared+rebase.
523+
// Fall back to multi-memory, which is sound (LS-D-1: emit correct
524+
// output, never a plausible-but-wrong one). Explicit
525+
// `--memory shared --address-rebase` remains available as an
526+
// opt-in, and warns loudly (see `warn_if_unsound_rebasing`).
527+
log::warn!(
528+
"memory strategy auto: {memory_count} memories, no memory.grow — \
529+
shared-memory fusion would apply here, but address rebasing is \
530+
unsound for computed memory addresses (#326); selecting \
531+
multi-memory instead. Use `--memory shared --address-rebase` to \
532+
override explicitly (unsound; corrupts computed-pointer access)."
501533
);
502-
self.config.memory_strategy = MemoryStrategy::SharedMemory;
503-
self.config.address_rebasing = true;
534+
self.config.memory_strategy = MemoryStrategy::MultiMemory;
535+
self.config.address_rebasing = false;
504536
}
505537
}
506538

meld-core/tests/auto_memory.rs

Lines changed: 47 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
1-
//! `MemoryStrategy::Auto` resolution — integration oracle for #172.
1+
//! `MemoryStrategy::Auto` resolution — integration oracle for #172 / #326.
22
//!
3-
//! The default memory strategy is `Auto`: shared memory + address rebasing
4-
//! when no input module contains `memory.grow` and there are at least two
5-
//! memories to merge, multi-memory otherwise. The point of the feature is
6-
//! that out-of-the-box `meld fuse` output flows through `wasm-opt → synth`
7-
//! with no extra flags, WITHOUT ever selecting the shared form where it is
8-
//! unsound (merger Bug #7: shared memory breaks under `memory.grow`).
3+
//! The default memory strategy is `Auto`. It resolves to **multi-memory**:
4+
//! shared memory + address rebasing was previously auto-selected for grow-free
5+
//! multi-memory inputs, but that path is UNSOUND (#326 — rebasing does not
6+
//! relocate the dynamic address operand of ordinary loads/stores, so
7+
//! computed-pointer access silently collides across components), so Auto no
8+
//! longer selects it. Shared+rebase remains reachable only via explicit
9+
//! `--memory shared --address-rebase`, which warns loudly. (Historically Auto
10+
//! also always avoided shared under `memory.grow` — merger Bug #7.)
911
//!
1012
//! The oracle here mirrors the issue's repro: `wasm-opt` (and any
1113
//! standards-default validator) rejects multi-memory modules. So the fused
@@ -19,9 +21,6 @@ use wasm_encoder::{
1921
ExportKind, ExportSection, Function, FunctionSection, Instruction, MemorySection, MemoryType,
2022
Module, ModuleSection, TypeSection,
2123
};
22-
use wasmtime::{Engine, Instance, Module as RuntimeModule, Store};
23-
24-
const WASM_PAGE_SIZE: usize = 65_536;
2524

2625
fn build_component(module: Module) -> Vec<u8> {
2726
let mut component = Component::new();
@@ -178,52 +177,33 @@ fn validates_without_multimemory(wasm: &[u8]) -> bool {
178177
.is_ok()
179178
}
180179

181-
/// Two grow-free components, one memory each → Auto must pick shared +
182-
/// rebasing: single-memory output, accepted by a no-flags validator, and
183-
/// behaviourally correct (B's data lands at its rebased base, A's intact).
180+
/// #326: address rebasing does not relocate the dynamic address operand of
181+
/// ordinary loads/stores, so shared-memory fusion silently corrupts any
182+
/// component that addresses memory via a computed pointer. Auto therefore no
183+
/// longer selects shared+rebase even for grow-free inputs — it falls back to
184+
/// multi-memory, the sound strategy (LS-D-1: correct output, never a
185+
/// plausible-but-wrong one). Shared remains reachable only via explicit
186+
/// `--memory shared --address-rebase`, which warns loudly.
184187
#[test]
185-
fn auto_selects_shared_for_growfree_inputs() {
188+
fn auto_gates_shared_for_growfree_inputs_326() {
186189
let component_a = build_component(build_module_a());
187190
let component_b = build_component(build_module_b(false));
188191

189192
let (fused, stats) = fuse_default(&[component_a, component_b]);
190193

191-
assert_eq!(stats.memory_strategy, "shared");
194+
assert_eq!(
195+
stats.memory_strategy, "multi",
196+
"auto must gate the unsound shared+rebase path (#326) and select multi"
197+
);
192198
assert_eq!(
193199
output_memory_count(&fused),
194-
1,
195-
"auto-resolved shared fusion must produce a single memory"
200+
2,
201+
"multi-memory keeps each component's memory separate — no silent \
202+
cross-component collision (the correctness #326 protects)"
196203
);
197204
assert!(
198-
validates_without_multimemory(&fused),
199-
"fused output must validate without --enable-multimemory (#172)"
200-
);
201-
202-
// Behaviour: the single memory holds A's fill at 0 and B's data one
203-
// page in (B's rebased base).
204-
let engine = Engine::default();
205-
let module = RuntimeModule::new(&engine, &fused).unwrap();
206-
let mut store = Store::new(&engine, ());
207-
let instance = Instance::new(&mut store, &module, &[]).unwrap();
208-
209-
instance
210-
.get_typed_func::<(), ()>(&mut store, "a_fill")
211-
.unwrap()
212-
.call(&mut store, ())
213-
.unwrap();
214-
instance
215-
.get_typed_func::<(), ()>(&mut store, "b_init")
216-
.unwrap()
217-
.call(&mut store, ())
218-
.unwrap();
219-
220-
let memory = instance.get_memory(&mut store, "memory").unwrap();
221-
let data = memory.data(&store);
222-
assert_eq!(&data[0..4], &[0x11, 0x11, 0x11, 0x11], "A's region");
223-
assert_eq!(
224-
&data[WASM_PAGE_SIZE..WASM_PAGE_SIZE + 4],
225-
&[1, 2, 3, 4],
226-
"B's region must sit at its rebased base, not clobber A"
205+
!validates_without_multimemory(&fused),
206+
"sanity: the multi-memory form is the one a no-flags validator rejects"
227207
);
228208
}
229209

@@ -265,11 +245,12 @@ fn auto_keeps_multi_for_single_memory_input() {
265245
}
266246

267247
/// Mythos finding A (PR #220): Auto resolution must be re-derived from the
268-
/// CURRENT component set on every fuse. A fuse → `add_component` → fuse
269-
/// sequence must not reuse the first fuse's stale "shared" resolution when
270-
/// the newly added component grows memory — the second fuse must succeed
271-
/// and resolve to multi. (`ls_m_7_` prefix: LS-M-7 / UCA-M-11 regression,
272-
/// run by the LS-N verification gate.)
248+
/// CURRENT component set on every fuse. Post-#326 Auto always resolves to
249+
/// multi-memory (shared+rebase is gated off as unsound), so re-probing is
250+
/// verified by the memory layout re-deriving: a grow-free pair fuses to 2
251+
/// memories, and after adding a third component the next fuse re-derives to
252+
/// 3 — not a stale 2. (`ls_m_7_` prefix: LS-M-7 / UCA-M-11 regression, run by
253+
/// the LS-N verification gate.)
273254
#[test]
274255
fn ls_m_7_auto_reprobes_after_add_component() {
275256
let component_a = build_component(build_module_a());
@@ -280,21 +261,29 @@ fn ls_m_7_auto_reprobes_after_add_component() {
280261
fuser.add_component_named(&component_a, Some("a")).unwrap();
281262
fuser.add_component_named(&component_b, Some("b")).unwrap();
282263

283-
let (_, stats) = fuser.fuse_with_stats().unwrap();
284-
assert_eq!(stats.memory_strategy, "shared", "grow-free pair → shared");
264+
let (fused1, stats) = fuser.fuse_with_stats().unwrap();
265+
assert_eq!(
266+
stats.memory_strategy, "multi",
267+
"grow-free pair → multi (#326 gates the unsound shared+rebase path)"
268+
);
269+
assert_eq!(
270+
output_memory_count(&fused1),
271+
2,
272+
"grow-free pair fuses to 2 memories"
273+
);
285274

286275
fuser
287276
.add_component_named(&component_grow, Some("grower"))
288277
.unwrap();
289278
let (fused, stats) = fuser
290279
.fuse_with_stats()
291-
.expect("second fuse must re-resolve, not reuse the stale shared plan");
280+
.expect("second fuse must re-resolve against the current component set");
281+
assert_eq!(stats.memory_strategy, "multi");
292282
assert_eq!(
293-
stats.memory_strategy, "multi",
294-
"a growing component added after a previous fuse must flip the \
295-
resolution back to multi"
283+
output_memory_count(&fused),
284+
3,
285+
"the added component must be re-probed: 3 memories, not a stale 2"
296286
);
297-
assert_eq!(output_memory_count(&fused), 3);
298287
}
299288

300289
/// Explicit strategies are untouched by Auto: `MultiMemory` still produces

meld-core/tests/cross_component_call.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,10 +169,18 @@ fn test_304_identity_direct_adapter_is_inlined() {
169169
return;
170170
};
171171

172+
// Identity-adapter inlining (#304) is exercised on the shared-memory
173+
// fusion path. Request it explicitly: since #326, `auto` no longer selects
174+
// shared+rebase (it is unsound for computed-pointer memory access), so this
175+
// feature test must opt into shared itself. The unsoundness #326 gates is
176+
// about memory *access*, not adapter inlining, so exercising shared here to
177+
// check the inlining + validity is fine.
172178
let mut fuser = Fuser::new(FuserConfig {
173179
attestation: false,
174180
reproducible: false,
175181
component_provenance: false,
182+
memory_strategy: MemoryStrategy::SharedMemory,
183+
address_rebasing: true,
176184
..Default::default()
177185
});
178186
fuser

0 commit comments

Comments
 (0)