Make limiter growth callbacks unwind-safe and kind-aware - #8
Open
kmatasfp wants to merge 1 commit into
Open
Conversation
GOL-424: `memory_grown` ran inside `LocalMemory::grow`, after the backing allocation had committed but before the owning `VMContext` was refreshed. Since the callback runs arbitrary embedder code, a panic caught there left compiled wasm observing stale bounds, or a `VMContext` pointing at a freed allocation when growth relocated the base. `LocalMemory::grow` no longer notifies. `Memory::grow` returns the old and new sizes and callers emit the notification once the state that unwinding could otherwise strand is in place: `Instance::memory_grow` after publishing the refreshed `VMMemoryDefinition`, and `StoreOpaque::grow_gc_heap` after `TakenGcHeap` has returned the grown memory and its size delta to the collector -- the DRC collector feeds that delta straight to `FreeList::add_capacity`, so notifying earlier could strand the new capacity. GOL-425: `memory_growing` fired for every `MemoryKind` but `memory_grown` only for `LinearMemory`, so a successful GC heap growth left an embedder holding a reservation it made in `memory_growing` and could never release. `memory_growing`, `memory_grown` and `memory_grow_failed` now all carry a public `MemoryKind`, so every permitted growth is resolved exactly once with the kind it was requested under, and GC heap capacity is never mistaken for guest linear memory. Regression tests in `runtime::limits::tests` cover the panicking callback for fixed-base and relocating linear memory and for the DRC GC heap, GC heap growth that succeeds, is rejected, and fails in the allocator, and ordinary linear-memory growth. Each was confirmed to fail against the previous behavior. They live here rather than in `tests/all/limits.rs` because `tests/all` does not currently compile on this branch, for unrelated reasons; that file is still updated for the new signatures and passes locally.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements GOL-424 and GOL-425.
Summary
VMContextholds the new base pointer and length, so a panicking limiter cannot leave a store inconsistentMemoryKindtomemory_growing,memory_grown, andmemory_grow_failedso embedders can account for guest linear memory separately from Wasmtime's internal GC heapmemory_grownforMemoryKind::GcHeapas well, so every permitted growth is resolved exactly onceBehavior
LocalMemory::growno longer notifies the limiter. It returns the old and new sizes, and each caller emits the notification once the state an unwind could otherwise strand is in place.Instance::memory_growdoes so after publishing the refreshedVMMemoryDefinition;StoreOpaque::grow_gc_heapdoes so afterTakenGcHeaphas put the grown memory anddelta_bytes_grownback into theGcStore. The deferred reference-counting collector feeds that delta straight toFreeList::add_capacity, so notifying earlier could strand the new capacity and force an immediate second growth.Because
memory_grownruns arbitrary embedder code, this matters whenever that code can panic and the panic is caught — Golem's callback is intended to be infallible but takes mutexes with.unwrap(). Previously a caught panic could leave compiled Wasm observing stale bounds, or aVMContextpointing at an allocation that growth had already relocated and freed. After the change the memory reports its new size, the host and guest views of it agree, and the instance and store remain usable.The three growth callbacks now carry the kind of heap being grown. Previously
memory_growingfired for everyMemoryKindwhilememory_grownfired only forLinearMemory, so a successful GC heap growth left an embedder holding a reservation made inmemory_growingthat it could never release; repeated GC growth eroded admission headroom until the store unloaded. Every permitted growth is now resolved by exactly onememory_grownormemory_grow_failedcarrying the kind it was requested under, and GC heap capacity is never presented as guest linear memory.Initial allocations still call
memory_growingwithout a resolving callback, and a request the memory's type cannot represent is refused before the limiter is consulted, somemory_grow_failedcan arrive unpaired. Both are pre-existing and now stated explicitly in the trait docs; with the kind available an embedder can decline to reserve forGcHeapat all. A store only allocates a GC heap when thegcfeature is compiled in,Config::wasm_gcis enabled, and something actually allocates a GC object — embedders that leave GC off will never observeMemoryKind::GcHeap.Success, rejection, and
memory_grow_failedbehavior is otherwise unchanged for non-GC memories, and shared memories continue to receive no callbacks.Testing
Regression tests live in
runtime::limits::testsrather thantests/all/limits.rsbecausetests/alldoes not currently compile on this branch, for reasons unrelated to this change (host_funcs.rsandtraps.rsreference a removedwasmtime_wasi::p1API).tests/all/limits.rsis still updated for the new signatures and passes locally with that breakage patched out. Each new test was confirmed to fail against the previous behavior.Golem
ResourceLimiterimplementation must take the newMemoryKindparameter, and should skipMemoryKind::GcHeapwhen reserving and releasing linear-memory admission grants