Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable limited stack overflow checks while running inside continuations #136

Merged
merged 4 commits into from
Mar 21, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions crates/cli-flags/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,9 @@ wasmtime_option_group! {
pub timeout: Option<Duration>,
/// Size of stacks created with cont.new instructions
pub wasmfx_stack_size: Option<usize>,
/// Space that must be left on stack when starting execution of a
/// function while running on a continuation stack.
pub wasmfx_red_zone_size: Option<usize>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

presumably this is platform/abi dependent? Like SysV mandates 16 bytes alignment. I suppose we round up when actually allocate the stack. Nonetheless, it may be worth writing here that the size given here is an underapproximation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed elsewhere, this value does not influence the allocation of stacks; it isn't really constrained at all in that regard. The only condition is that it should be smaller than the stack size. I've added a comment stating this condition, even though we don't currently check it.

/// Configures support for all WebAssembly proposals implemented.
pub all_proposals: Option<bool>,
/// Configure support for the bulk memory proposal.
Expand Down Expand Up @@ -552,6 +555,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]
Expand Down
10 changes: 10 additions & 0 deletions crates/continuations/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -46,6 +52,10 @@ 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.
pub red_zone_size: usize,
}

/// This type is used to save (and subsequently restore) a subset of the data in
Expand Down
18 changes: 9 additions & 9 deletions crates/runtime/src/continuation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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,
Expand Down Expand Up @@ -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())
}

Expand Down
6 changes: 1 addition & 5 deletions crates/runtime/src/traphandlers/backtrace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 8 additions & 0 deletions crates/wasmtime/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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.
///
Expand Down
45 changes: 45 additions & 0 deletions tests/all/typed_continuations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::<Trap>());
assert_eq!(*error.downcast_ref::<Trap>().unwrap(), Trap::StackOverflow);

Ok(())
}
}