Fix GC heap using wrong MemoryKind tunables in on-demand allocator (#13189)

* Fix GC heap using wrong `MemoryKind` tunables in on-demand allocator

The on-demand instance allocator's `allocate_memory` always passed
`MemoryKind::LinearMemory` to `Memory::new_dynamic`, even when
allocating the backing memory for a GC heap. This caused the GC heap to
use the wrong set of tunables.

The consequences were two distinct crash modes:

1. When `memory_reservation=0`: the GC heap got a 0-byte reservation, so every
   growth triggered an mmap reallocation that changed the base pointer. Code
   held a stale cached copy of the old base pointer, causing use-after-free
   crashes.

2. When `gc_heap_guard_size != memory_guard_size`: Cranelift correctly compiled
   GC heap accesses using `gc_heap_guard_size` bytes of virtual guard, but the
   runtime memory incorrectly only had `memory_guard_size` bytes of actual
   guard, so accesses beyond the actual guard caused uncaught segfaults.

This commit fixes the bug by threading a `MemoryKind` through the
`InstanceAllocator::allocate_memory` trait method.

Also, now that the GC heap correctly uses `gc_heap_reservation`, the workaround
in `MemoryTunables::may_move` that always returned `true` for GC heaps is no
longer needed. That workaround was masking the actual root cause, and with the
fix, `gc_heap_may_move` can now be respected correctly.

There were three existing tests that were accidentally relying on the GC heap
using linear-memory tunables; this commit adds explicit GC heap tunable
configuration so they continue to work correctly.

Fixes #13173

* Fix compiler warning

Author: fitzgen

crates/environ/src/tunables.rs

@@ -392,13 +392,7 @@ impl<'a> MemoryTunables<'a> {
     pub fn may_move(&self) -> bool {
         match self.kind {
             MemoryKind::LinearMemory => self.tunables.memory_may_move,
-            MemoryKind::GcHeap => {
-                // XXX: Lazy GC heap allocation means that the GC heap's base
-                // may always "move" in that it is initially null and then gets
-                // initialized after being lazily allocated.
-                let _ = self.tunables.gc_heap_may_move;
-                true
-            }
+            MemoryKind::GcHeap => self.tunables.gc_heap_may_move,
         }
     }
 

crates/wasmtime/src/runtime/store.rs

@@ -1969,7 +1969,12 @@ impl StoreOpaque {
 
             let (mem_alloc_index, mem) = engine
                 .allocator()
-                .allocate_memory(&mut request, &mem_ty, None)
+                .allocate_memory(
+                    &mut request,
+                    &mem_ty,
+                    None,
+                    wasmtime_environ::MemoryKind::GcHeap,
+                )
                 .await?;
 
             // Then, allocate the actual GC heap, passing in that memory

crates/wasmtime/src/runtime/trampoline/memory.rs

@@ -12,8 +12,8 @@ use alloc::sync::Arc;
 use core::future::Future;
 use core::pin::Pin;
 use wasmtime_environ::{
-    DefinedMemoryIndex, DefinedTableIndex, EntityIndex, HostPtr, MemoryTunables, Module,
-    StaticModuleIndex, VMOffsets,
+    DefinedMemoryIndex, DefinedTableIndex, EntityIndex, HostPtr, MemoryKind, MemoryTunables,
+    Module, StaticModuleIndex, VMOffsets,
 };
 
 #[cfg(feature = "component-model")]
@@ -176,6 +176,7 @@ unsafe impl InstanceAllocator for SingleMemoryInstance<'_> {
         request: &'a mut InstanceAllocationRequest<'b, 'c>,
         ty: &'a wasmtime_environ::Memory,
         memory_index: Option<DefinedMemoryIndex>,
+        memory_kind: MemoryKind,
     ) -> Pin<Box<dyn Future<Output = Result<(MemoryAllocationIndex, Memory)>> + Send + 'a>> {
         if cfg!(debug_assertions) {
             let module = request.runtime_info.env_module();
@@ -191,7 +192,9 @@ unsafe impl InstanceAllocator for SingleMemoryInstance<'_> {
                     shared_memory.clone().as_memory(),
                 ))
             }),
-            None => self.ondemand.allocate_memory(request, ty, memory_index),
+            None => self
+                .ondemand
+                .allocate_memory(request, ty, memory_index, memory_kind),
         }
     }
 

crates/wasmtime/src/runtime/vm/instance/allocator.rs

@@ -14,8 +14,8 @@ use core::pin::Pin;
 use core::{mem, ptr};
 use wasmtime_environ::{
     DefinedMemoryIndex, DefinedTableIndex, EntityRef, HostPtr, InitMemory, MemoryInitialization,
-    MemoryInitializer, Module, SizeOverflow, TableInitialValue, TableSegmentElements, Trap,
-    VMOffsets, WasmRefType,
+    MemoryInitializer, MemoryKind, Module, SizeOverflow, TableInitialValue, TableSegmentElements,
+    Trap, VMOffsets, WasmRefType,
 };
 
 #[cfg(feature = "gc")]
@@ -195,6 +195,7 @@ pub unsafe trait InstanceAllocator: Send + Sync {
         request: &'a mut InstanceAllocationRequest<'b, 'c>,
         ty: &'a wasmtime_environ::Memory,
         memory_index: Option<DefinedMemoryIndex>,
+        memory_kind: MemoryKind,
     ) -> Pin<Box<dyn Future<Output = Result<(MemoryAllocationIndex, Memory)>> + Send + 'a>>;
 
     /// Deallocate an instance's previously allocated memory.
@@ -418,7 +419,7 @@ impl dyn InstanceAllocator + '_ {
                 .expect("should be a defined memory since we skipped imported ones");
 
             let memory = self
-                .allocate_memory(request, ty, Some(memory_index))
+                .allocate_memory(request, ty, Some(memory_index), MemoryKind::LinearMemory)
                 .await?;
             memories.push(memory)?;
         }

crates/wasmtime/src/runtime/vm/instance/allocator/on_demand.rs

@@ -10,7 +10,9 @@ use crate::runtime::vm::table::Table;
 use alloc::sync::Arc;
 use core::future::Future;
 use core::pin::Pin;
-use wasmtime_environ::{DefinedMemoryIndex, DefinedTableIndex, HostPtr, Module, VMOffsets};
+use wasmtime_environ::{
+    DefinedMemoryIndex, DefinedTableIndex, HostPtr, MemoryKind, Module, VMOffsets,
+};
 
 #[cfg(feature = "gc")]
 use crate::runtime::vm::{GcHeap, GcHeapAllocationIndex, GcRuntime};
@@ -115,7 +117,10 @@ unsafe impl InstanceAllocator for OnDemandInstanceAllocator {
         request: &'a mut InstanceAllocationRequest<'b, 'c>,
         ty: &'a wasmtime_environ::Memory,
         memory_index: Option<DefinedMemoryIndex>,
+        memory_kind: MemoryKind,
     ) -> Pin<Box<dyn Future<Output = Result<(MemoryAllocationIndex, Memory)>> + Send + 'a>> {
+        debug_assert_eq!(memory_index.is_none(), memory_kind == MemoryKind::GcHeap);
+
         let creator = self
             .mem_creator
             .as_deref()
@@ -135,6 +140,7 @@ unsafe impl InstanceAllocator for OnDemandInstanceAllocator {
                 creator,
                 image,
                 request.limiter.as_deref_mut(),
+                memory_kind,
             )
             .await?;
             Ok((allocation_index, memory))

crates/wasmtime/src/runtime/vm/instance/allocator/pooling.rs

@@ -66,7 +66,7 @@ use std::{
     sync::atomic::{AtomicU64, Ordering},
 };
 use wasmtime_environ::{
-    DefinedMemoryIndex, DefinedTableIndex, HostPtr, Module, Tunables, VMOffsets,
+    DefinedMemoryIndex, DefinedTableIndex, HostPtr, MemoryKind, Module, Tunables, VMOffsets,
 };
 
 pub use self::metrics::PoolingAllocatorMetrics;
@@ -688,6 +688,7 @@ unsafe impl InstanceAllocator for PoolingInstanceAllocator {
         request: &'a mut InstanceAllocationRequest<'b, 'c>,
         ty: &'a wasmtime_environ::Memory,
         memory_index: Option<DefinedMemoryIndex>,
+        _memory_kind: MemoryKind,
     ) -> Pin<Box<dyn Future<Output = Result<(MemoryAllocationIndex, Memory)>> + Send + 'a>> {
         crate::runtime::box_future(async move {
             async {

crates/wasmtime/src/runtime/vm/memory.rs

@@ -244,10 +244,11 @@ impl Memory {
         creator: &dyn RuntimeMemoryCreator,
         memory_image: Option<&Arc<MemoryImage>>,
         limiter: Option<&mut StoreResourceLimiter<'_>>,
+        kind: MemoryKind,
     ) -> Result<Self> {
         let (minimum, maximum) = Self::limit_new(ty, limiter).await?;
         let tunables = engine.tunables();
-        let memory_tunables = MemoryTunables::new(tunables, MemoryKind::LinearMemory);
+        let memory_tunables = MemoryTunables::new(tunables, kind);
         let allocation = creator.new_memory(ty, &memory_tunables, minimum, maximum)?;
 
         let memory = LocalMemory::new(ty, &memory_tunables, allocation, memory_image)?;

tests/all/gc.rs

@@ -1581,6 +1581,9 @@ fn instantiate_global_init_oom() -> Result<()> {
     config.memory_may_move(false);
     config.memory_reservation(64 << 10);
     config.memory_reservation_for_growth(0);
+    config.gc_heap_may_move(false);
+    config.gc_heap_reservation(64 << 10);
+    config.gc_heap_reservation_for_growth(0);
     let engine = Engine::new(&config)?;
     let mut store = Store::new(&engine, ());
 
@@ -1611,6 +1614,9 @@ fn elem_const_eval_oom() -> Result<()> {
     config.memory_may_move(false);
     config.memory_reservation(64 << 10);
     config.memory_reservation_for_growth(0);
+    config.gc_heap_may_move(false);
+    config.gc_heap_reservation(64 << 10);
+    config.gc_heap_reservation_for_growth(0);
     let engine = Engine::new(&config)?;
     let mut store = Store::new(&engine, ());
 
@@ -1845,7 +1851,7 @@ fn issue_13141_gc_heap_may_not_move() -> Result<()> {
     let mut config = Config::new();
     config.wasm_gc(true);
     config.gc_heap_may_move(false);
-    config.gc_heap_reservation(0);
+
     let engine = Engine::new(&config)?;
 
     let module = Module::new(
@@ -2023,3 +2029,119 @@ fn issue_13037_drc_leak_passing_objects_already_in_over_approx_stack_roots_list(
 
     Ok(())
 }
+
+#[test]
+#[cfg_attr(miri, ignore)]
+fn issue_13173_gc_heap_uses_gc_tunables_no_signals() -> Result<()> {
+    let _ = env_logger::try_init();
+
+    let mut config = Config::new();
+    config.wasm_gc(true);
+    config.wasm_function_references(true);
+    config.collector(Collector::DeferredReferenceCounting);
+    // Set `memory_reservation=0` while leaving `gc_heap_reservation` at its
+    // default (4GB on 64-bit). Before the fix, the GC heap would incorrectly
+    // use `memory_reservation=0`, giving it 0 capacity and causing segfaults
+    // as the heap base pointer changed on every growth.
+    config.memory_reservation(0);
+    config.signals_based_traps(false);
+
+    let engine = Engine::new(&config)?;
+
+    let module = Module::new(
+        &engine,
+        r#"
+        (module
+            (type $arr (array (mut i32)))
+            (func (export "run") (result i32)
+                (local $i i32)
+                (local $a (ref $arr))
+                (local.set $a (array.new_default $arr (i32.const 64)))
+                (block $break
+                    (loop $loop
+                        (br_if $break (i32.ge_u (local.get $i) (i32.const 1000)))
+                        (array.set $arr
+                            (local.get $a)
+                            (i32.const 0)
+                            (i32.add
+                                (array.get $arr (local.get $a) (i32.const 0))
+                                (i32.const 1)))
+                        (local.set $i (i32.add (local.get $i) (i32.const 1)))
+                        (br $loop)
+                    )
+                )
+                (array.get $arr (local.get $a) (i32.const 0))
+            )
+        )
+        "#,
+    )?;
+
+    let mut store = Store::new(&engine, ());
+    let instance = Instance::new(&mut store, &module, &[])?;
+    let run = instance.get_typed_func::<(), i32>(&mut store, "run")?;
+    let result = run.call(&mut store, ())?;
+    assert_eq!(result, 1000);
+    Ok(())
+}
+
+#[test]
+#[cfg_attr(miri, ignore)]
+fn issue_13173_gc_heap_uses_gc_tunables_guard_size_mismatch() -> Result<()> {
+    if std::mem::size_of::<usize>() < std::mem::size_of::<u64>()
+        || std::env::var("WASMTIME_TEST_NO_HOG_MEMORY").is_ok()
+    {
+        return Ok(());
+    }
+
+    let _ = env_logger::try_init();
+
+    let mut config = Config::new();
+    config.wasm_gc(true);
+    config.wasm_function_references(true);
+    config.collector(Collector::DeferredReferenceCounting);
+    // Set `memory_reservation=0` and a large `gc_heap_guard_size` while leaving
+    // `memory_guard_size` at its default. Before the fix, the GC heap would use
+    // `memory_guard_size` (small) instead of `gc_heap_guard_size` (large), causing
+    // Cranelift-generated code that relies on virtual memory protection for
+    // accesses within the gc_heap_guard_size to segfault.
+    config.memory_reservation(0);
+    config.gc_heap_guard_size(1 << 29); // 512MB GC heap guard
+    config.memory_guard_size(32 * 1024 * 1024); // 32MB linear memory guard
+
+    let engine = Engine::new(&config)?;
+
+    let module = Module::new(
+        &engine,
+        r#"
+        (module
+            (type $arr (array (mut i32)))
+            (func (export "run") (result i32)
+                (local $i i32)
+                (local $a (ref $arr))
+                (local.set $a (array.new_default $arr (i32.const 64)))
+                (block $break
+                    (loop $loop
+                        (br_if $break (i32.ge_u (local.get $i) (i32.const 1000)))
+                        (array.set $arr
+                            (local.get $a)
+                            (i32.const 0)
+                            (i32.add
+                                (array.get $arr (local.get $a) (i32.const 0))
+                                (i32.const 1)))
+                        (local.set $i (i32.add (local.get $i) (i32.const 1)))
+                        (br $loop)
+                    )
+                )
+                (array.get $arr (local.get $a) (i32.const 0))
+            )
+        )
+        "#,
+    )?;
+
+    let mut store = Store::new(&engine, ());
+    let instance = Instance::new(&mut store, &module, &[])?;
+    let run = instance.get_typed_func::<(), i32>(&mut store, "run")?;
+    let result = run.call(&mut store, ())?;
+    assert_eq!(result, 1000);
+    Ok(())
+}