From d6522e27279408c12feff57e84c479ce0ab6ac4b Mon Sep 17 00:00:00 2001 From: Frank Emrich Date: Thu, 21 Mar 2024 13:29:45 +0000 Subject: [PATCH] Enable limited stack overflow checks while running inside continuations (#136) Currently, we can overflow the stack while running inside a continuation, without the runtime having any way of detecting this. This PR partially rectifies this, by making the existing stack limit checks that get emitted by cranelift in every wasm function prelude work correctly while running inside a continuation. All that was required to enable the stack limit checks was the following: 1. Stop zero-ing out the `stack_limit` value in `VMRuntimeLimits` whenever we `resume` a continuation. 2. When creating a continuation, set a reasonable value for the `stack_limits` value in its `StackLimits` object. Note that all the required infrastructure to make sure that whenever we switch stacks, we save and restore the `stack_limits` value inside `VMRuntimeLimits` and the `StackLimits` object of the involved stacks was already implemented in #98 and #99. In this sense, enabling these checks is "free": The limits were already checked, but previously using a limit of 0. The only remaining question is what the "reasonable value" for the stack limits value mentioned above is. As discussed in #122, the stack limit checks that cranelift emits in function preludes are rather limited, and these limitations are reflected in the checks that this PR provides: When entering a wasm function, they check that the current stack pointer is larger than the `stack_limit` value in `VMRuntimeLimits`. They do not take into account how much stack space the function itself will occupy. No stack limit checks are performed when calling a host function. Thus, this PR defines a config option `wasmfx_red_zone_size`. The idea is that we define the stack limit as `bottom_of_fiber_stack` + `wasmfx_red_zone_size`. Thus, the stack checks boil down to the following: Whenever we enter a wasm function while inside a continuation, we ensure that there are at least `wasmfx_red_zone_size` bytes of stack space left. I've set the default value for `wasmfx_red_zone_size` to 32k. To get a rough idea for a sensible value, I determined that a call to the `fd_write` WASI function occupies ~21k of stack space, and generously rounded this up to 32k. **Important**: This means that these stack limit checks are incomplete: Calling a wasm or host function that occupies more than `wasmfx_red_zone_size` of stack space may still result in an undetected stack overflow! --- crates/cli-flags/src/lib.rs | 7 +++ crates/continuations/src/lib.rs | 11 +++++ crates/runtime/src/continuation.rs | 18 ++++---- crates/runtime/src/traphandlers/backtrace.rs | 6 +-- crates/wasmtime/src/config.rs | 8 ++++ tests/all/typed_continuations.rs | 45 ++++++++++++++++++++ 6 files changed, 81 insertions(+), 14 deletions(-) diff --git a/crates/cli-flags/src/lib.rs b/crates/cli-flags/src/lib.rs index f4397880310d..5a0ccea676c1 100644 --- a/crates/cli-flags/src/lib.rs +++ b/crates/cli-flags/src/lib.rs @@ -211,6 +211,10 @@ wasmtime_option_group! { pub timeout: Option, /// Size of stacks created with cont.new instructions pub wasmfx_stack_size: Option, + /// Space that must be left on stack when starting execution of a + /// function while running on a continuation stack. + /// Must be smaller than the `wasmfx_stack_size` option above. + pub wasmfx_red_zone_size: Option, /// Configures support for all WebAssembly proposals implemented. pub all_proposals: Option, /// Configure support for the bulk memory proposal. @@ -552,6 +556,9 @@ impl CommonOptions { if let Some(wasmfx_stack_size) = self.wasm.wasmfx_stack_size { config.wasmfx_stack_size(wasmfx_stack_size); } + if let Some(wasmfx_red_zone_size) = self.wasm.wasmfx_red_zone_size { + config.wasmfx_red_zone_size(wasmfx_red_zone_size); + } match_feature! { ["pooling-allocator" : self.opts.pooling_allocator] diff --git a/crates/continuations/src/lib.rs b/crates/continuations/src/lib.rs index 62ebedfa14f0..960927e71cf9 100644 --- a/crates/continuations/src/lib.rs +++ b/crates/continuations/src/lib.rs @@ -3,6 +3,12 @@ use std::ptr; /// Default size for continuation stacks pub const DEFAULT_FIBER_SIZE: usize = 2097152; // 2MB = 512 pages of 4k +/// Default size of the red zone at the bottom of a fiber stack. This means that +/// whenever we are executing on a fiber stack and starting (!) execution of a +/// wasm (!) function, the stack pointer must be at least this many bytes away +/// from the bottom of the fiber stack. +pub const DEFAULT_RED_ZONE_SIZE: usize = 32768; // 32K = 8 pages of 4k size + /// TODO #[allow(dead_code)] pub const ENABLE_DEBUG_PRINTING: bool = false; @@ -46,6 +52,11 @@ pub mod types { #[derive(Clone)] pub struct WasmFXConfig { pub stack_size: usize, + + /// Space that must be left on stack when starting execution of a + /// function while running on a continuation stack. + /// Must be smaller than the value of `stack_size` above. + pub red_zone_size: usize, } /// This type is used to save (and subsequently restore) a subset of the data in diff --git a/crates/runtime/src/continuation.rs b/crates/runtime/src/continuation.rs index 5f0642507ff2..aea828826a29 100644 --- a/crates/runtime/src/continuation.rs +++ b/crates/runtime/src/continuation.rs @@ -314,9 +314,14 @@ pub fn cont_new( let capacity = cmp::max(param_count, result_count); let payload = Payloads::new(capacity); + let wasmfx_config = unsafe { &*(*instance.store()).wasmfx_config() }; + // TODO(frank-emrich) Currently, the general `stack_limit` configuration + // option of wasmtime is unrelated to the stack size of our fiber stack. + let stack_size = wasmfx_config.stack_size; + let red_zone_size = wasmfx_config.red_zone_size; + let fiber = { - let wasmfx_config = unsafe { &*(*instance.store()).wasmfx_config() }; - let stack = FiberStack::malloc(wasmfx_config.stack_size) + let stack = FiberStack::malloc(stack_size) .map_err(|error| TrapReason::user_without_backtrace(error.into()))?; let args_ptr = payload.data; let fiber = Fiber::new(stack, move |_first_val: (), _suspend: &Yield| unsafe { @@ -332,8 +337,9 @@ pub fn cont_new( }; let tsp = fiber.stack().top().unwrap(); + let stack_limit = unsafe { tsp.sub(stack_size - red_zone_size) } as usize; let contobj = Box::new(ContinuationObject { - limits: StackLimits::with_stack_limit(unsafe { tsp.sub(DEFAULT_FIBER_SIZE) } as usize), + limits: StackLimits::with_stack_limit(stack_limit), fiber: Box::into_raw(fiber), parent_chain: StackChain::Absent, args: payload, @@ -413,12 +419,6 @@ pub fn resume( *runtime_limits.last_wasm_entry_sp.get() = (*contobj).limits.last_wasm_entry_sp; } - unsafe { - (*(*(*instance.store()).vmruntime_limits()) - .stack_limit - .get_mut()) = 0 - }; - Ok(fiber.resume()) } diff --git a/crates/runtime/src/traphandlers/backtrace.rs b/crates/runtime/src/traphandlers/backtrace.rs index 6c612b186202..c7fc3348ea68 100644 --- a/crates/runtime/src/traphandlers/backtrace.rs +++ b/crates/runtime/src/traphandlers/backtrace.rs @@ -230,11 +230,7 @@ impl Backtrace { let stack_range = (*cont.fiber).stack().range().unwrap(); debug_assert!(stack_range.contains(&limits.last_wasm_exit_fp)); debug_assert!(stack_range.contains(&limits.last_wasm_entry_sp)); - // TODO(frank-emrich) Enable this assertion once we stop - // zero-ing the stack limit in - // `wasmtime_runtime::continuation::resume` - // - // debug_assert_eq!(stack_range.end, limits.stack_limit); + debug_assert!(stack_range.contains(&limits.stack_limit)); }, None => { // reached stack information for main stack diff --git a/crates/wasmtime/src/config.rs b/crates/wasmtime/src/config.rs index b836fff96fa7..4fd29999e9a1 100644 --- a/crates/wasmtime/src/config.rs +++ b/crates/wasmtime/src/config.rs @@ -223,6 +223,7 @@ impl Config { compiler_config: CompilerConfig::default(), wasmfx_config: WasmFXConfig { stack_size: wasmtime_continuations::DEFAULT_FIBER_SIZE, + red_zone_size: wasmtime_continuations::DEFAULT_RED_ZONE_SIZE, }, #[cfg(feature = "cache")] cache_config: CacheConfig::new_cache_disabled(), @@ -699,6 +700,13 @@ impl Config { self } + /// Configures the amount of space that must be left on stack when starting + /// execution of a function while running on a continuation stack. + pub fn wasmfx_red_zone_size(&mut self, size: usize) -> &mut Self { + self.wasmfx_config.red_zone_size = size; + self + } + /// Configures whether the WebAssembly tail calls proposal will be enabled /// for compilation or not. /// diff --git a/tests/all/typed_continuations.rs b/tests/all/typed_continuations.rs index a5ab92a9f130..ca9fd2246059 100644 --- a/tests/all/typed_continuations.rs +++ b/tests/all/typed_continuations.rs @@ -1116,4 +1116,49 @@ mod traps { Ok(()) } + + #[test] + #[cfg_attr(feature = "typed_continuations_baseline_implementation", ignore)] + fn stack_overflow_in_continuation() -> Result<()> { + let wat = r#" + (module + (type $ft (func (param i32))) + (type $ct (cont $ft)) + + (func $entry (export "entry") + (call $a) + ) + + (func $a (export "a") + ;; We ask for a billion recursive calls + (i32.const 1_000_000_000) + + (resume $ct (cont.new $ct (ref.func $overflow))) + ) + + (func $overflow (export "overflow") (param $i i32) + (block $continue + (local.get $i) + ;; return if $i == 0 + (br_if $continue) + (return) + ) + (i32.sub (local.get $i) (i32.const 1)) + (call $overflow) + ) + + ) + "#; + + let runner = Runner::new(); + + let error = runner + .run_test::<()>(wat, &[]) + .expect_err("Expecting execution to yield error"); + + assert!(error.root_cause().is::()); + assert_eq!(*error.downcast_ref::().unwrap(), Trap::StackOverflow); + + Ok(()) + } }