From de469e2790f0c33f99df483f2dae4ceb47e11c8e Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 9 Oct 2024 13:53:06 -0500 Subject: [PATCH] Change stack walking to stop at a precise fp (#9420) * Change stack walking to stop at a precise fp Prior to this commit entry trampolines into wasm would record their stack pointer at the time of the function call to wasm and then this stack pointer was used to halt the stack walking process. The problem with this though is that due to the `tail` ABI it's possible that the callee will update the caller's stack pointer temporarily. This means that the recorded stack pointer at the time the trampoline called wasm may differ from the callee's idea of what the stack pointer is when a backtrace happens. To handle this condition when stack walking the frame pointer instead of the stack pointer is now recorded when wasm is invoked. This frame pointer is a trusted value as it's managed by Cranelift itself. This additionally enables the stop condition for frame walking to be a precise "it must be this value" condition. Put together this commit fixes an issue where when `return_call` is used it's possible for the initial few frames of the stack to get lost in stack traces. After this the frame pointer chain should always be precisely walked in its entirety, even in the face of different numbers of arguments and parameters as `return_call` instructions are executed. * Add tail-calls, params, and results to stacks fuzzer This commit extends the preexisting `stacks` fuzzer with a few new features: * `return_call` instructions are now generated * functions may have both params/results to exercise logic around stack adjustments and how that might affect a stack trace --- crates/cranelift/src/compiler.rs | 10 +- crates/environ/src/vmoffsets.rs | 4 +- crates/fuzzing/src/generators/stacks.rs | 136 +++++++++++++----- crates/fuzzing/src/oracles/stacks.rs | 5 +- .../wasmtime/src/runtime/vm/arch/aarch64.rs | 8 -- crates/wasmtime/src/runtime/vm/arch/mod.rs | 8 -- .../wasmtime/src/runtime/vm/arch/riscv64.rs | 8 -- crates/wasmtime/src/runtime/vm/arch/s390x.rs | 8 -- .../src/runtime/vm/arch/unsupported.rs | 8 -- crates/wasmtime/src/runtime/vm/arch/x86_64.rs | 8 -- .../wasmtime/src/runtime/vm/traphandlers.rs | 12 +- .../src/runtime/vm/traphandlers/backtrace.rs | 122 +++++++--------- crates/wasmtime/src/runtime/vm/vmcontext.rs | 8 +- tests/all/traps.rs | 66 +++++++++ 14 files changed, 236 insertions(+), 175 deletions(-) diff --git a/crates/cranelift/src/compiler.rs b/crates/cranelift/src/compiler.rs index ee62289d4307..487ece1ca972 100644 --- a/crates/cranelift/src/compiler.rs +++ b/crates/cranelift/src/compiler.rs @@ -280,7 +280,7 @@ impl wasmtime_environ::Compiler for Compiler { debug_assert_vmctx_kind(isa, &mut builder, vmctx, wasmtime_environ::VMCONTEXT_MAGIC); let offsets = VMOffsets::new(isa.pointer_bytes(), &translation.module); let vm_runtime_limits_offset = offsets.ptr.vmctx_runtime_limits(); - save_last_wasm_entry_sp( + save_last_wasm_entry_fp( &mut builder, pointer_type, &offsets.ptr, @@ -969,7 +969,7 @@ fn debug_assert_vmctx_kind( } } -fn save_last_wasm_entry_sp( +fn save_last_wasm_entry_fp( builder: &mut FunctionBuilder, pointer_type: ir::Type, ptr_size: &impl PtrSize, @@ -985,12 +985,12 @@ fn save_last_wasm_entry_sp( ); // Then store our current stack pointer into the appropriate slot. - let sp = builder.ins().get_stack_pointer(pointer_type); + let fp = builder.ins().get_frame_pointer(pointer_type); builder.ins().store( MemFlags::trusted(), - sp, + fp, limits, - ptr_size.vmruntime_limits_last_wasm_entry_sp(), + ptr_size.vmruntime_limits_last_wasm_entry_fp(), ); } diff --git a/crates/environ/src/vmoffsets.rs b/crates/environ/src/vmoffsets.rs index 7bf02b5954af..8b4accffed6d 100644 --- a/crates/environ/src/vmoffsets.rs +++ b/crates/environ/src/vmoffsets.rs @@ -172,8 +172,8 @@ pub trait PtrSize { self.vmruntime_limits_last_wasm_exit_fp() + self.size() } - /// Return the offset of the `last_wasm_entry_sp` field of `VMRuntimeLimits`. - fn vmruntime_limits_last_wasm_entry_sp(&self) -> u8 { + /// Return the offset of the `last_wasm_entry_fp` field of `VMRuntimeLimits`. + fn vmruntime_limits_last_wasm_entry_fp(&self) -> u8 { self.vmruntime_limits_last_wasm_exit_pc() + self.size() } diff --git a/crates/fuzzing/src/generators/stacks.rs b/crates/fuzzing/src/generators/stacks.rs index 4aada9a150d5..e7a3fcb31f34 100644 --- a/crates/fuzzing/src/generators/stacks.rs +++ b/crates/fuzzing/src/generators/stacks.rs @@ -8,10 +8,11 @@ use std::mem; use arbitrary::{Arbitrary, Result, Unstructured}; -use wasm_encoder::Instruction; +use wasm_encoder::{Instruction, ValType}; -const MAX_FUNCS: usize = 20; +const MAX_FUNCS: u32 = 20; const MAX_OPS: usize = 1_000; +const MAX_PARAMS: usize = 10; /// Generate a Wasm module that keeps track of its current call stack, to /// compare to the host. @@ -24,13 +25,16 @@ pub struct Stacks { #[derive(Debug, Default)] struct Function { ops: Vec, + params: usize, + results: usize, } -#[derive(Arbitrary, Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy)] enum Op { CheckStackInHost, Call(u32), CallThroughHost(u32), + ReturnCall(u32), } impl<'a> Arbitrary<'a> for Stacks { @@ -44,35 +48,44 @@ impl<'a> Arbitrary<'a> for Stacks { impl Stacks { fn arbitrary_funcs(u: &mut Unstructured) -> Result> { - let mut funcs = vec![Function::default()]; - - // The indices of functions within `funcs` that we still need to - // generate. - let mut work_list = vec![0]; + // Generate a list of functions first with a number of parameters and + // results. Bodies are generated afterwards. + let nfuncs = u.int_in_range(1..=MAX_FUNCS)?; + let mut funcs = (0..nfuncs) + .map(|_| { + Ok(Function { + ops: Vec::new(), // generated later + params: u.int_in_range(0..=MAX_PARAMS)?, + results: u.int_in_range(0..=MAX_PARAMS)?, + }) + }) + .collect::>>()?; + let mut funcs_by_result = vec![Vec::new(); MAX_PARAMS + 1]; + for (i, func) in funcs.iter().enumerate() { + funcs_by_result[func.results].push(i as u32); + } - while let Some(f) = work_list.pop() { - let mut ops = Vec::with_capacity(u.arbitrary_len::()?.min(MAX_OPS)); - for _ in 0..ops.capacity() { - ops.push(u.arbitrary()?); - } - for op in &mut ops { - match op { - Op::CallThroughHost(idx) | Op::Call(idx) => { - if u.is_empty() || funcs.len() >= MAX_FUNCS || u.ratio(4, 5)? { - // Call an existing function. - *idx = *idx % u32::try_from(funcs.len()).unwrap(); - } else { - // Call a new function... - *idx = u32::try_from(funcs.len()).unwrap(); - // ...which means we also need to eventually define it. - work_list.push(funcs.len()); - funcs.push(Function::default()); - } - } - Op::CheckStackInHost => {} + // Fill in each function body with various instructions/operations now + // that the set of functions is known. + for f in funcs.iter_mut() { + let funcs_with_same_results = &funcs_by_result[f.results]; + for _ in 0..u.arbitrary_len::()?.min(MAX_OPS) { + let op = match u.int_in_range(0..=3)? { + 0 => Op::CheckStackInHost, + 1 => Op::Call(u.int_in_range(0..=nfuncs - 1)?), + 2 => Op::CallThroughHost(u.int_in_range(0..=nfuncs - 1)?), + // This only works if the target function has the same + // number of results, so choose from a different set here. + 3 => Op::ReturnCall(*u.choose(funcs_with_same_results)?), + _ => unreachable!(), + }; + f.ops.push(op); + // once a `return_call` has been generated there's no need to + // generate any more instructions, so fall through to below. + if let Some(Op::ReturnCall(_)) = f.ops.last() { + break; } } - funcs[f].ops = ops; } Ok(funcs) @@ -120,14 +133,22 @@ impl Stacks { vec![wasm_encoder::ValType::I32, wasm_encoder::ValType::I32], ); - let null_type = types.len(); - types.ty().function(vec![], vec![]); - let call_func_type = types.len(); types .ty() .function(vec![wasm_encoder::ValType::FUNCREF], vec![]); + let check_stack_type = types.len(); + types.ty().function(vec![], vec![]); + + let func_types_start = types.len(); + for func in self.funcs.iter() { + types.ty().function( + vec![ValType::I32; func.params], + vec![ValType::I32; func.results], + ); + } + section(&mut module, types); let mut imports = wasm_encoder::ImportSection::new(); @@ -135,7 +156,7 @@ impl Stacks { imports.import( "host", "check_stack", - wasm_encoder::EntityType::Function(null_type), + wasm_encoder::EntityType::Function(check_stack_type), ); let call_func_func = 1; imports.import( @@ -147,8 +168,8 @@ impl Stacks { section(&mut module, imports); let mut funcs = wasm_encoder::FunctionSection::new(); - for _ in &self.funcs { - funcs.function(null_type); + for (i, _) in self.funcs.iter().enumerate() { + funcs.function(func_types_start + (i as u32)); } let run_func = funcs.len() + num_imported_funcs; funcs.function(run_type); @@ -246,6 +267,25 @@ impl Stacks { .instruction(&Instruction::GlobalSet(stack_len_global)); }; + let push_params = |body: &mut wasm_encoder::Function, func: u32| { + let func = &self.funcs[func as usize]; + for _ in 0..func.params { + body.instruction(&Instruction::I32Const(0)); + } + }; + let pop_results = |body: &mut wasm_encoder::Function, func: u32| { + let func = &self.funcs[func as usize]; + for _ in 0..func.results { + body.instruction(&Instruction::Drop); + } + }; + let push_results = |body: &mut wasm_encoder::Function, func: u32| { + let func = &self.funcs[func as usize]; + for _ in 0..func.results { + body.instruction(&Instruction::I32Const(0)); + } + }; + let mut code = wasm_encoder::CodeSection::new(); for (func_index, func) in self.funcs.iter().enumerate() { let mut body = wasm_encoder::Function::new(vec![]); @@ -256,19 +296,35 @@ impl Stacks { ); check_fuel(&mut body); + let mut check_fuel_and_pop_at_end = true; + // Perform our specified operations. for op in &func.ops { + assert!(check_fuel_and_pop_at_end); match op { Op::CheckStackInHost => { body.instruction(&Instruction::Call(check_stack_func)); } Op::Call(f) => { + push_params(&mut body, *f); body.instruction(&Instruction::Call(f + num_imported_funcs)); + pop_results(&mut body, *f); } Op::CallThroughHost(f) => { body.instruction(&Instruction::RefFunc(f + num_imported_funcs)) .instruction(&Instruction::Call(call_func_func)); } + + // For a `return_call` preemptively check fuel to possibly + // trap and then pop our function from the in-wasm managed + // stack. After that execute the `return_call` itself. + Op::ReturnCall(f) => { + push_params(&mut body, *f); + check_fuel(&mut body); + pop_func_from_stack(&mut body); + check_fuel_and_pop_at_end = false; + body.instruction(&Instruction::ReturnCall(f + num_imported_funcs)); + } } } @@ -278,9 +334,11 @@ impl Stacks { // function, but then we returned back to Wasm and then trapped // while `last_wasm_exit_sp` et al are still initialized from that // previous host call. - check_fuel(&mut body); - - pop_func_from_stack(&mut body); + if check_fuel_and_pop_at_end { + check_fuel(&mut body); + pop_func_from_stack(&mut body); + push_results(&mut body, func_index as u32); + } function(&mut code, body); } @@ -307,7 +365,9 @@ impl Stacks { check_fuel(&mut run_body); // Call the first locally defined function. + push_params(&mut run_body, 0); run_body.instruction(&Instruction::Call(num_imported_funcs)); + pop_results(&mut run_body, 0); check_fuel(&mut run_body); pop_func_from_stack(&mut run_body); diff --git a/crates/fuzzing/src/oracles/stacks.rs b/crates/fuzzing/src/oracles/stacks.rs index d3ce4ca9892f..01137bbe85b2 100644 --- a/crates/fuzzing/src/oracles/stacks.rs +++ b/crates/fuzzing/src/oracles/stacks.rs @@ -40,7 +40,10 @@ pub fn check_stacks(stacks: Stacks) -> usize { "call_func", |mut caller: Caller<'_, ()>, f: Option| { let f = f.unwrap(); - f.call(&mut caller, &[], &mut [])?; + let ty = f.ty(&caller); + let params = vec![Val::I32(0); ty.params().len()]; + let mut results = vec![Val::I32(0); ty.results().len()]; + f.call(&mut caller, ¶ms, &mut results)?; Ok(()) }, ) diff --git a/crates/wasmtime/src/runtime/vm/arch/aarch64.rs b/crates/wasmtime/src/runtime/vm/arch/aarch64.rs index 5c7ae5084deb..8d2e6daefd9c 100644 --- a/crates/wasmtime/src/runtime/vm/arch/aarch64.rs +++ b/crates/wasmtime/src/runtime/vm/arch/aarch64.rs @@ -50,14 +50,6 @@ pub unsafe fn get_next_older_pc_from_fp(fp: usize) -> usize { // And the current frame pointer points to the next older frame pointer. pub const NEXT_OLDER_FP_FROM_FP_OFFSET: usize = 0; -pub fn reached_entry_sp(fp: usize, entry_sp: usize) -> bool { - fp >= entry_sp -} - -pub fn assert_entry_sp_is_aligned(sp: usize) { - assert_eq!(sp % 16, 0, "stack should always be aligned to 16"); -} - pub fn assert_fp_is_aligned(_fp: usize) { // From AAPCS64, section 6.2.3 The Frame Pointer[0]: // diff --git a/crates/wasmtime/src/runtime/vm/arch/mod.rs b/crates/wasmtime/src/runtime/vm/arch/mod.rs index ea4b5e4f1f5a..adb346ceffe3 100644 --- a/crates/wasmtime/src/runtime/vm/arch/mod.rs +++ b/crates/wasmtime/src/runtime/vm/arch/mod.rs @@ -40,14 +40,6 @@ pub unsafe fn get_next_older_pc_from_fp(fp: usize) -> usize { pub const NEXT_OLDER_FP_FROM_FP_OFFSET: usize = imp::NEXT_OLDER_FP_FROM_FP_OFFSET; -pub fn reached_entry_sp(fp: usize, entry_sp: usize) -> bool { - imp::reached_entry_sp(fp, entry_sp) -} - -pub fn assert_entry_sp_is_aligned(sp: usize) { - imp::assert_entry_sp_is_aligned(sp) -} - pub fn assert_fp_is_aligned(fp: usize) { imp::assert_fp_is_aligned(fp) } diff --git a/crates/wasmtime/src/runtime/vm/arch/riscv64.rs b/crates/wasmtime/src/runtime/vm/arch/riscv64.rs index 8875b35592cd..0ca7f8209af6 100644 --- a/crates/wasmtime/src/runtime/vm/arch/riscv64.rs +++ b/crates/wasmtime/src/runtime/vm/arch/riscv64.rs @@ -21,14 +21,6 @@ pub unsafe fn get_next_older_pc_from_fp(fp: usize) -> usize { // And the current frame pointer points to the next older frame pointer. pub const NEXT_OLDER_FP_FROM_FP_OFFSET: usize = 0; -pub fn reached_entry_sp(fp: usize, entry_sp: usize) -> bool { - fp >= entry_sp -} - -pub fn assert_entry_sp_is_aligned(sp: usize) { - assert_eq!(sp % 16, 0, "stack should always be aligned to 16"); -} - pub fn assert_fp_is_aligned(fp: usize) { assert_eq!(fp % 16, 0, "stack should always be aligned to 16"); } diff --git a/crates/wasmtime/src/runtime/vm/arch/s390x.rs b/crates/wasmtime/src/runtime/vm/arch/s390x.rs index e2f53ecb4797..2773381243ab 100644 --- a/crates/wasmtime/src/runtime/vm/arch/s390x.rs +++ b/crates/wasmtime/src/runtime/vm/arch/s390x.rs @@ -17,14 +17,6 @@ pub unsafe fn get_next_older_pc_from_fp(fp: usize) -> usize { // by the current "FP". pub const NEXT_OLDER_FP_FROM_FP_OFFSET: usize = 0; -pub fn reached_entry_sp(fp: usize, entry_sp: usize) -> bool { - fp > entry_sp -} - -pub fn assert_entry_sp_is_aligned(sp: usize) { - assert_eq!(sp % 8, 0, "stack should always be aligned to 8"); -} - pub fn assert_fp_is_aligned(fp: usize) { assert_eq!(fp % 8, 0, "stack should always be aligned to 8"); } diff --git a/crates/wasmtime/src/runtime/vm/arch/unsupported.rs b/crates/wasmtime/src/runtime/vm/arch/unsupported.rs index d4073fbaecb6..7f7759fa4a77 100644 --- a/crates/wasmtime/src/runtime/vm/arch/unsupported.rs +++ b/crates/wasmtime/src/runtime/vm/arch/unsupported.rs @@ -31,14 +31,6 @@ pub unsafe fn get_next_older_pc_from_fp(_fp: usize) -> usize { pub const NEXT_OLDER_FP_FROM_FP_OFFSET: usize = 0; -pub fn reached_entry_sp(_fp: usize, _entry_sp: usize) -> bool { - panic!() -} - -pub fn assert_entry_sp_is_aligned(_sp: usize) { - panic!() -} - pub fn assert_fp_is_aligned(_fp: usize) { panic!() } diff --git a/crates/wasmtime/src/runtime/vm/arch/x86_64.rs b/crates/wasmtime/src/runtime/vm/arch/x86_64.rs index cd47c4a4b0b2..f8091400bfe9 100644 --- a/crates/wasmtime/src/runtime/vm/arch/x86_64.rs +++ b/crates/wasmtime/src/runtime/vm/arch/x86_64.rs @@ -23,14 +23,6 @@ pub unsafe fn get_next_older_pc_from_fp(fp: usize) -> usize { // And the current frame pointer points to the next older frame pointer. pub const NEXT_OLDER_FP_FROM_FP_OFFSET: usize = 0; -pub fn reached_entry_sp(fp: usize, entry_sp: usize) -> bool { - fp >= entry_sp -} - -pub fn assert_entry_sp_is_aligned(sp: usize) { - assert_eq!(sp % 16, 0, "stack should always be aligned to 16"); -} - pub fn assert_fp_is_aligned(fp: usize) { assert_eq!(fp % 16, 0, "stack should always be aligned to 16"); } diff --git a/crates/wasmtime/src/runtime/vm/traphandlers.rs b/crates/wasmtime/src/runtime/vm/traphandlers.rs index 0f405c03ee8a..d9b9a326e857 100644 --- a/crates/wasmtime/src/runtime/vm/traphandlers.rs +++ b/crates/wasmtime/src/runtime/vm/traphandlers.rs @@ -323,7 +323,7 @@ mod call_thread_state { // contiguous-Wasm-frames stack regions for backtracing purposes. old_last_wasm_exit_fp: Cell, old_last_wasm_exit_pc: Cell, - old_last_wasm_entry_sp: Cell, + old_last_wasm_entry_fp: Cell, } impl Drop for CallThreadState { @@ -331,7 +331,7 @@ mod call_thread_state { unsafe { *(*self.limits).last_wasm_exit_fp.get() = self.old_last_wasm_exit_fp.get(); *(*self.limits).last_wasm_exit_pc.get() = self.old_last_wasm_exit_pc.get(); - *(*self.limits).last_wasm_entry_sp.get() = self.old_last_wasm_entry_sp.get(); + *(*self.limits).last_wasm_entry_fp.get() = self.old_last_wasm_entry_fp.get(); } } } @@ -359,7 +359,7 @@ mod call_thread_state { prev: Cell::new(ptr::null()), old_last_wasm_exit_fp: Cell::new(unsafe { *(*limits).last_wasm_exit_fp.get() }), old_last_wasm_exit_pc: Cell::new(unsafe { *(*limits).last_wasm_exit_pc.get() }), - old_last_wasm_entry_sp: Cell::new(unsafe { *(*limits).last_wasm_entry_sp.get() }), + old_last_wasm_entry_fp: Cell::new(unsafe { *(*limits).last_wasm_entry_fp.get() }), } } @@ -373,9 +373,9 @@ mod call_thread_state { self.old_last_wasm_exit_pc.get() } - /// Get the saved SP upon entry into Wasm for the previous `CallThreadState`. - pub fn old_last_wasm_entry_sp(&self) -> usize { - self.old_last_wasm_entry_sp.get() + /// Get the saved FP upon entry into Wasm for the previous `CallThreadState`. + pub fn old_last_wasm_entry_fp(&self) -> usize { + self.old_last_wasm_entry_fp.get() } /// Get the previous `CallThreadState`. diff --git a/crates/wasmtime/src/runtime/vm/traphandlers/backtrace.rs b/crates/wasmtime/src/runtime/vm/traphandlers/backtrace.rs index 0c610a35252c..21e2b1975288 100644 --- a/crates/wasmtime/src/runtime/vm/traphandlers/backtrace.rs +++ b/crates/wasmtime/src/runtime/vm/traphandlers/backtrace.rs @@ -125,7 +125,7 @@ impl Backtrace { let activations = core::iter::once(( last_wasm_exit_pc, last_wasm_exit_fp, - *(*limits).last_wasm_entry_sp.get(), + *(*limits).last_wasm_entry_fp.get(), )) .chain( state @@ -135,7 +135,7 @@ impl Backtrace { ( state.old_last_wasm_exit_pc(), state.old_last_wasm_exit_fp(), - state.old_last_wasm_entry_sp(), + state.old_last_wasm_entry_fp(), ) }), ) @@ -162,11 +162,11 @@ impl Backtrace { unsafe fn trace_through_wasm( mut pc: usize, mut fp: usize, - trampoline_sp: usize, + trampoline_fp: usize, mut f: impl FnMut(Frame) -> ControlFlow<()>, ) -> ControlFlow<()> { log::trace!("=== Tracing through contiguous sequence of Wasm frames ==="); - log::trace!("trampoline_sp = 0x{:016x}", trampoline_sp); + log::trace!("trampoline_fp = 0x{:016x}", trampoline_fp); log::trace!(" initial pc = 0x{:016x}", pc); log::trace!(" initial fp = 0x{:016x}", fp); @@ -174,41 +174,60 @@ impl Backtrace { // caller. assert_ne!(pc, 0); assert_ne!(fp, 0); - assert_ne!(trampoline_sp, 0); + assert_ne!(trampoline_fp, 0); - arch::assert_entry_sp_is_aligned(trampoline_sp); - - // It is possible that the contiguous sequence of Wasm frames is - // empty. This is rare, but can happen if: + // This loop will walk the linked list of frame pointers starting at + // `fp` and going up until `trampoline_fp`. We know that both `fp` and + // `trampoline_fp` are "trusted values" aka generated and maintained by + // Cranelift. This means that it should be safe to walk the linked list + // of pointers and inspect wasm frames. // - // * Host calls into Wasm, pushing the entry trampoline frame + // Note, though, that any frames outside of this range are not + // guaranteed to have valid frame pointers. For example native code + // might be using the frame pointer as a general purpose register. Thus + // we need to be careful to only walk frame pointers in this one + // contiguous linked list. // - // * Entry trampoline calls the actual Wasm function, pushing a Wasm frame + // To know when to stop iteration all architectures' stacks currently + // look something like this: // - // * Wasm function tail calls to an imported host function, *replacing* - // the Wasm frame with the exit trampoline's frame + // | ... | + // | Native Frames | + // | ... | + // |-------------------| + // | ... | <-- Trampoline FP | + // | Trampoline Frame | | + // | ... | <-- Trampoline SP | + // |-------------------| Stack + // | Return Address | Grows + // | Previous FP | <-- Wasm FP Down + // | ... | | + // | Wasm Frames | | + // | ... | V // - // Now we have a stack like `[host, entry trampoline, exit trampoline]` - // which has a contiguous sequence of Wasm frames that are empty. + // The trampoline records its own frame pointer (`trampoline_fp`), + // which is guaranteed to be above all Wasm. To check when we've + // reached the trampoline frame, it is therefore sufficient to + // check when the next frame pointer is equal to `trampoline_fp`. Once + // that's hit then we know that the entire linked list has been + // traversed. // - // Therefore, check if we've reached the entry trampoline's SP as the - // first thing we do. - if arch::reached_entry_sp(fp, trampoline_sp) { - log::trace!("=== Empty contiguous sequence of Wasm frames ==="); - return ControlFlow::Continue(()); - } - - loop { + // Note that it might be possible that this loop doesn't execute at all. + // For example if the entry trampoline called wasm which `return_call`'d + // an imported function which is an exit trampoline, then + // `fp == trampoline_fp` on the entry of this function, meaning the loop + // won't actually execute anything. + while fp != trampoline_fp { // At the start of each iteration of the loop, we know that `fp` is // a frame pointer from Wasm code. Therefore, we know it is not // being used as an extra general-purpose register, and it is safe // dereference to get the PC and the next older frame pointer. - - // The stack grows down, and therefore any frame pointer we are - // dealing with should be less than the stack pointer on entry - // to Wasm. - assert!(trampoline_sp >= fp, "{trampoline_sp:#x} >= {fp:#x}"); - + // + // The stack also grows down, and therefore any frame pointer we are + // dealing with should be less than the frame pointer on entry to + // Wasm. Finally also assert that it's aligned correctly as an + // additional sanity check. + assert!(trampoline_fp > fp, "{trampoline_fp:#x} > {fp:#x}"); arch::assert_fp_is_aligned(fp); log::trace!("--- Tracing through one Wasm frame ---"); @@ -227,55 +246,16 @@ impl Backtrace { // Get the next older frame pointer from the current Wasm frame // pointer. - // - // The next older frame pointer may or may not be a Wasm frame's - // frame pointer, but it is trusted either way (i.e. is actually a - // frame pointer and not being used as a general-purpose register) - // because we always enter Wasm from the host via a trampoline, and - // this trampoline maintains a proper frame pointer. - // - // We want to detect when we've reached the trampoline, and break - // out of this stack-walking loop. All of our architectures' stacks - // grow down and look something vaguely like this: - // - // | ... | - // | Native Frames | - // | ... | - // |-------------------| - // | ... | <-- Trampoline FP | - // | Trampoline Frame | | - // | ... | <-- Trampoline SP | - // |-------------------| Stack - // | Return Address | Grows - // | Previous FP | <-- Wasm FP Down - // | ... | | - // | Wasm Frames | | - // | ... | V - // - // The trampoline records its own stack pointer (`trampoline_sp`), - // which is guaranteed to be above all Wasm frame pointers but at or - // below its own frame pointer. It is usually two words above the - // Wasm frame pointer (at least on x86-64, exact details vary across - // architectures) but not always: if the first Wasm function called - // by the host has many arguments, some of them could be passed on - // the stack in between the return address and the trampoline's - // frame. - // - // To check when we've reached the trampoline frame, it is therefore - // sufficient to check when the next frame pointer is greater than - // or equal to `trampoline_sp` (except s390x, where it needs to be - // strictly greater than). let next_older_fp = *(fp as *mut usize).add(arch::NEXT_OLDER_FP_FROM_FP_OFFSET); - if arch::reached_entry_sp(next_older_fp, trampoline_sp) { - log::trace!("=== Done tracing contiguous sequence of Wasm frames ==="); - return ControlFlow::Continue(()); - } // Because the stack always grows down, the older FP must be greater // than the current FP. assert!(next_older_fp > fp, "{next_older_fp:#x} > {fp:#x}"); fp = next_older_fp; } + + log::trace!("=== Done tracing contiguous sequence of Wasm frames ==="); + ControlFlow::Continue(()) } /// Iterate over the frames inside this backtrace. diff --git a/crates/wasmtime/src/runtime/vm/vmcontext.rs b/crates/wasmtime/src/runtime/vm/vmcontext.rs index 59ae7ba0fbf0..ddc33e468906 100644 --- a/crates/wasmtime/src/runtime/vm/vmcontext.rs +++ b/crates/wasmtime/src/runtime/vm/vmcontext.rs @@ -834,7 +834,7 @@ pub struct VMRuntimeLimits { /// /// Used to find the end of a contiguous sequence of Wasm frames when /// walking the stack. - pub last_wasm_entry_sp: UnsafeCell, + pub last_wasm_entry_fp: UnsafeCell, } // The `VMRuntimeLimits` type is a pod-type with no destructor, and we don't @@ -852,7 +852,7 @@ impl Default for VMRuntimeLimits { epoch_deadline: UnsafeCell::new(0), last_wasm_exit_fp: UnsafeCell::new(0), last_wasm_exit_pc: UnsafeCell::new(0), - last_wasm_entry_sp: UnsafeCell::new(0), + last_wasm_entry_fp: UnsafeCell::new(0), } } } @@ -888,8 +888,8 @@ mod test_vmruntime_limits { usize::from(offsets.ptr.vmruntime_limits_last_wasm_exit_pc()) ); assert_eq!( - offset_of!(VMRuntimeLimits, last_wasm_entry_sp), - usize::from(offsets.ptr.vmruntime_limits_last_wasm_entry_sp()) + offset_of!(VMRuntimeLimits, last_wasm_entry_fp), + usize::from(offsets.ptr.vmruntime_limits_last_wasm_entry_fp()) ); } } diff --git a/tests/all/traps.rs b/tests/all/traps.rs index f5368a48f4a7..23144cbb10f4 100644 --- a/tests/all/traps.rs +++ b/tests/all/traps.rs @@ -1790,3 +1790,69 @@ fn return_call_indirect_to_imported_function(config: &mut Config) -> Result<()> Ok(()) } + +#[test] +fn return_call_to_aborting_wasm_function_with_stack_adjustments() -> Result<()> { + let engine = Engine::default(); + let module = Module::new( + &engine, + r#" +(module + (func (export "entry") + (param i64 i64 i64 i64 i64 i64) + (result i64 i64 i64 i64 i64 i64 i64 i64 i64 i64) + return_call $abort + ) + (func $abort (result i64 i64 i64 i64 i64 i64 i64 i64 i64 i64) + unreachable + ) +) + "#, + )?; + let mut store = Store::new(&engine, ()); + let instance = Instance::new(&mut store, &module, &[])?; + let func = instance.get_func(&mut store, "entry").unwrap(); + let args = vec![Val::I64(0); 6]; + let mut results = vec![Val::I64(0); 10]; + + let err = func.call(&mut store, &args, &mut results).unwrap_err(); + + let trap: &Trap = err.downcast_ref().unwrap(); + assert_eq!(*trap, Trap::UnreachableCodeReached); + + let trace: &WasmBacktrace = err.downcast_ref().unwrap(); + assert_eq!(trace.frames().len(), 1); + assert_eq!(trace.frames()[0].func_name(), Some("abort")); + + let module2 = Module::new( + &engine, + r#" +(module + (func (export "entry") + (param i64 i64 i64 i64 i64 i64) + (result i64 i64 i64 i64 i64 i64 i64 i64 i64 i64) + return_call $foo + ) + (func $foo (result i64 i64 i64 i64 i64 i64 i64 i64 i64 i64) + call $abort + unreachable + ) + (func $abort unreachable) +) + "#, + )?; + let instance = Instance::new(&mut store, &module2, &[])?; + let func = instance.get_func(&mut store, "entry").unwrap(); + + let err = func.call(&mut store, &args, &mut results).unwrap_err(); + + let trap: &Trap = err.downcast_ref().unwrap(); + assert_eq!(*trap, Trap::UnreachableCodeReached); + + let trace: &WasmBacktrace = err.downcast_ref().unwrap(); + assert_eq!(trace.frames().len(), 2); + assert_eq!(trace.frames()[0].func_name(), Some("abort")); + assert_eq!(trace.frames()[1].func_name(), Some("foo")); + + Ok(()) +}