Change proc_exit to unwind the stack rather than exiting the host process. (#1646)

* Remove Cranelift's OutOfBounds trap, which is no longer used.

* Change proc_exit to unwind instead of exit the host process.

This implements the semantics in https://github.com/WebAssembly/WASI/pull/235.

Fixes #783.
Fixes #993.

* Fix exit-status tests on Windows.

* Revert the wiggle changes and re-introduce the wasi-common implementations.

* Move `wasi_proc_exit` into the wasmtime-wasi crate.

* Revert the spec_testsuite change.

* Remove the old proc_exit implementations.

* Make `TrapReason` an implementation detail.

* Allow exit status 2 on Windows too.

* Fix a documentation link.

* Really fix a documentation link.
This commit is contained in:
Dan Gohman
2020-05-13 15:59:43 -07:00
committed by GitHub
parent 08983bf39c
commit fb0b9e3ae6
27 changed files with 268 additions and 64 deletions

View File

@@ -27,9 +27,6 @@ pub enum TrapCode {
/// A `table_addr` instruction detected an out-of-bounds error.
TableOutOfBounds,
/// Other bounds checking error.
OutOfBounds,
/// Indirect call to a null table entry.
IndirectCallToNull,
@@ -63,7 +60,6 @@ impl Display for TrapCode {
StackOverflow => "stk_ovf",
HeapOutOfBounds => "heap_oob",
TableOutOfBounds => "table_oob",
OutOfBounds => "oob",
IndirectCallToNull => "icall_null",
BadSignature => "bad_sig",
IntegerOverflow => "int_ovf",
@@ -86,7 +82,6 @@ impl FromStr for TrapCode {
"stk_ovf" => Ok(StackOverflow),
"heap_oob" => Ok(HeapOutOfBounds),
"table_oob" => Ok(TableOutOfBounds),
"oob" => Ok(OutOfBounds),
"icall_null" => Ok(IndirectCallToNull),
"bad_sig" => Ok(BadSignature),
"int_ovf" => Ok(IntegerOverflow),
@@ -106,11 +101,10 @@ mod tests {
use alloc::string::ToString;
// Everything but user-defined codes.
const CODES: [TrapCode; 11] = [
const CODES: [TrapCode; 10] = [
TrapCode::StackOverflow,
TrapCode::HeapOutOfBounds,
TrapCode::TableOutOfBounds,
TrapCode::OutOfBounds,
TrapCode::IndirectCallToNull,
TrapCode::BadSignature,
TrapCode::IntegerOverflow,

View File

@@ -25,7 +25,7 @@ block201:
return
block202:
trap oob
trap heap_oob
}
; check: v1 = ifcmp_imm v0, 17
; check: brif eq v1, block201
@@ -56,7 +56,7 @@ block201:
return
block202:
trap oob
trap heap_oob
}
; check: v2 = ffcmp v0, v1
; check: brff eq v2, block201

View File

@@ -223,7 +223,7 @@ function %cond_traps(i32) {
block0(v0: i32):
trapz v0, stk_ovf
v1 = ifcmp_imm v0, 5
trapif ugt v1, oob
trapif ugt v1, heap_oob
v2 = bitcast.f32 v1
v3 = ffcmp v2, v2
trapff uno v3, int_ovf
@@ -233,7 +233,7 @@ block0(v0: i32):
; nextln: block0(v0: i32):
; nextln: trapz v0, stk_ovf
; nextln: v1 = ifcmp_imm v0, 5
; nextln: trapif ugt v1, oob
; nextln: trapif ugt v1, heap_oob
; nextln: v2 = bitcast.f32 v1
; nextln: v3 = ffcmp v2, v2
; nextln: trapff uno v3, int_ovf

View File

@@ -139,7 +139,7 @@ block0(v0: i64):
jump block4
block4:
trap oob
trap heap_oob
block2:
v8 = load.i64 v5+8

View File

@@ -54,7 +54,7 @@ block0(v0: i32):
br_table v0, block4, jt0
block4:
trap oob
trap heap_oob
block1:
return

View File

@@ -342,13 +342,6 @@ pub(crate) struct FdEventData<'a> {
pub(crate) userdata: wasi::__wasi_userdata_t,
}
pub(crate) fn proc_exit(_wasi_ctx: &WasiCtx, _memory: &mut [u8], rval: wasi::__wasi_exitcode_t) {
trace!("proc_exit(rval={:?})", rval);
// TODO: Rather than call std::process::exit here, we should trigger a
// stack unwind similar to a trap.
std::process::exit(rval as i32);
}
pub(crate) fn proc_raise(
_wasi_ctx: &WasiCtx,
_memory: &mut [u8],

View File

@@ -813,13 +813,10 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx {
Ok(nevents)
}
// This is just a temporary to ignore the warning which becomes a hard error
// in the CI. Once we figure out non-returns in `wiggle`, this should be gone.
#[allow(unreachable_code)]
fn proc_exit(&self, rval: types::Exitcode) -> std::result::Result<(), ()> {
// TODO: Rather than call std::process::exit here, we should trigger a
// stack unwind similar to a trap.
std::process::exit(rval as i32);
fn proc_exit(&self, _rval: types::Exitcode) -> std::result::Result<(), ()> {
// proc_exit is special in that it's expected to unwind the stack, which
// typically requires runtime-specific logic.
unimplemented!("runtimes are expected to override this implementation")
}
fn proc_raise(&self, _sig: types::Signal) -> Result<()> {

View File

@@ -17,6 +17,13 @@ pub fn define(args: TokenStream) -> TokenStream {
for module in doc.modules() {
for func in module.funcs() {
// `proc_exit` is special; it's essentially an unwinding primitive,
// so we implement it in the runtime rather than use the implementation
// in wasi-common.
if func.name.as_str() == "proc_exit" {
continue;
}
ret.extend(generate_wrappers(&func, old));
}
}

View File

@@ -46,6 +46,15 @@ pub fn define_struct(args: TokenStream) -> TokenStream {
linker_add.push(quote! {
linker.define(#module_name, #name, self.#name_ident.clone())?;
});
// `proc_exit` is special; it's essentially an unwinding primitive,
// so we implement it in the runtime rather than use the implementation
// in wasi-common.
if name == "proc_exit" {
ctor_externs.push(quote! {
let #name_ident = wasmtime::Func::wrap(store, crate::wasi_proc_exit);
});
continue;
}
let mut shim_arg_decls = Vec::new();
let mut params = Vec::new();
@@ -291,6 +300,15 @@ pub fn define_struct_for_wiggle(args: TokenStream) -> TokenStream {
linker_add.push(quote! {
linker.define(#module_name, #name, self.#name_ident.clone())?;
});
// `proc_exit` is special; it's essentially an unwinding primitive,
// so we implement it in the runtime rather than use the implementation
// in wasi-common.
if name == "proc_exit" {
ctor_externs.push(quote! {
let #name_ident = wasmtime::Func::wrap(store, crate::wasi_proc_exit);
});
continue;
}
let mut shim_arg_decls = Vec::new();
let mut params = Vec::new();

View File

@@ -1,3 +1,5 @@
use wasmtime::Trap;
pub mod old;
pub use wasi_common::{WasiCtx, WasiCtxBuilder};
@@ -12,3 +14,17 @@ pub fn is_wasi_module(name: &str) -> bool {
// trick.
name.starts_with("wasi")
}
/// Implement the WASI `proc_exit` function. This function is implemented here
/// instead of in wasi-common so that we can use the runtime to perform an
/// unwind rather than exiting the host process.
fn wasi_proc_exit(status: i32) -> Result<(), Trap> {
// Check that the status is within WASI's range.
if status >= 0 && status < 126 {
Err(Trap::i32_exit(status))
} else {
Err(Trap::new(
"exit with invalid exit status outside of [0..126)",
))
}
}

View File

@@ -420,7 +420,7 @@ impl Table {
let src_table = src_table.instance.get_defined_table(src_table_index);
runtime::Table::copy(dst_table, src_table, dst_index, src_index, len)
.map_err(Trap::from_jit)?;
.map_err(Trap::from_runtime)?;
Ok(())
}

View File

@@ -8,8 +8,8 @@ use std::mem;
use std::panic::{self, AssertUnwindSafe};
use std::ptr;
use std::rc::Weak;
use wasmtime_runtime::{raise_user_trap, ExportFunction, VMTrampoline};
use wasmtime_runtime::{Export, InstanceHandle, VMContext, VMFunctionBody};
use wasmtime_runtime::{ExportFunction, VMTrampoline};
/// A WebAssembly function which can be called.
///
@@ -747,7 +747,7 @@ pub(crate) fn catch_traps(
signalhandler.as_deref(),
closure,
)
.map_err(Trap::from_jit)
.map_err(Trap::from_runtime)
}
}
@@ -941,7 +941,7 @@ unsafe impl<T: WasmTy> WasmRet for Result<T, Trap> {
}
fn handle_trap(trap: Trap) -> ! {
unsafe { wasmtime_runtime::raise_user_trap(Box::new(trap)) }
unsafe { raise_user_trap(trap.into()) }
}
}
@@ -1133,9 +1133,9 @@ macro_rules! impl_into_func {
R::push(&mut ret);
let ty = FuncType::new(_args.into(), ret.into());
let store_weak = store.weak();
unsafe {
let trampoline = trampoline::<$($args,)* R>;
let (instance, export) = crate::trampoline::generate_raw_func_export(
let (instance, export) = unsafe {
crate::trampoline::generate_raw_func_export(
&ty,
std::slice::from_raw_parts_mut(
shim::<F, $($args,)* R> as *mut _,
@@ -1145,7 +1145,8 @@ macro_rules! impl_into_func {
store,
Box::new((self, store_weak)),
)
.expect("failed to generate export");
.expect("failed to generate export")
};
Func {
instance,
export,
@@ -1153,7 +1154,6 @@ macro_rules! impl_into_func {
}
}
}
}
)*)
}

View File

@@ -52,7 +52,7 @@ fn instantiate(
.map_err(|e| -> Error {
match e {
InstantiationError::StartTrap(trap) | InstantiationError::Trap(trap) => {
Trap::from_jit(trap).into()
Trap::from_runtime(trap).into()
}
other => other.into(),
}

View File

@@ -54,7 +54,7 @@ unsafe extern "C" fn stub_fn(
// If a trap was raised (an error returned from the imported function)
// then we smuggle the trap through `Box<dyn Error>` through to the
// call-site, which gets unwrapped in `Trap::from_jit` later on as we
// call-site, which gets unwrapped in `Trap::from_runtime` later on as we
// convert from the internal `Trap` type to our own `Trap` type in this
// crate.
Ok(Err(trap)) => wasmtime_runtime::raise_user_trap(Box::new(trap)),

View File

@@ -12,8 +12,27 @@ pub struct Trap {
inner: Arc<TrapInner>,
}
/// State describing the occasion which evoked a trap.
#[derive(Debug)]
enum TrapReason {
/// An error message describing a trap.
Message(String),
/// An `i32` exit status describing an explicit program exit.
I32Exit(i32),
}
impl fmt::Display for TrapReason {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
TrapReason::Message(s) => write!(f, "{}", s),
TrapReason::I32Exit(status) => write!(f, "Exited with i32 exit status {}", status),
}
}
}
struct TrapInner {
message: String,
reason: TrapReason,
wasm_trace: Vec<FrameInfo>,
native_trace: Backtrace,
}
@@ -34,9 +53,21 @@ impl Trap {
Trap::new_with_trace(&info, None, message.into(), Backtrace::new_unresolved())
}
pub(crate) fn from_jit(jit: wasmtime_runtime::Trap) -> Self {
/// Creates a new `Trap` representing an explicit program exit with a classic `i32`
/// exit status value.
pub fn i32_exit(status: i32) -> Self {
Trap {
inner: Arc::new(TrapInner {
reason: TrapReason::I32Exit(status),
wasm_trace: Vec::new(),
native_trace: Backtrace::from(Vec::new()),
}),
}
}
pub(crate) fn from_runtime(runtime_trap: wasmtime_runtime::Trap) -> Self {
let info = FRAME_INFO.read().unwrap();
match jit {
match runtime_trap {
wasmtime_runtime::Trap::User(error) => {
// Since we're the only one using the wasmtime internals (in
// theory) we should only see user errors which were originally
@@ -85,7 +116,6 @@ impl Trap {
StackOverflow => "call stack exhausted",
HeapOutOfBounds => "out of bounds memory access",
TableOutOfBounds => "undefined element: out of bounds table access",
OutOfBounds => "out of bounds",
IndirectCallToNull => "uninitialized element",
BadSignature => "indirect call type mismatch",
IntegerOverflow => "integer overflow",
@@ -128,7 +158,7 @@ impl Trap {
}
Trap {
inner: Arc::new(TrapInner {
message,
reason: TrapReason::Message(message),
wasm_trace,
native_trace,
}),
@@ -136,8 +166,23 @@ impl Trap {
}
/// Returns a reference the `message` stored in `Trap`.
///
/// In the case of an explicit exit, the exit status can be obtained by
/// calling `i32_exit_status`.
pub fn message(&self) -> &str {
&self.inner.message
match &self.inner.reason {
TrapReason::Message(message) => message,
TrapReason::I32Exit(_) => "explicitly exited",
}
}
/// If the trap was the result of an explicit program exit with a classic
/// `i32` exit status value, return the value, otherwise return `None`.
pub fn i32_exit_status(&self) -> Option<i32> {
match self.inner.reason {
TrapReason::I32Exit(status) => Some(status),
_ => None,
}
}
/// Returns a list of function frames in WebAssembly code that led to this
@@ -150,7 +195,7 @@ impl Trap {
impl fmt::Debug for Trap {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("Trap")
.field("message", &self.inner.message)
.field("reason", &self.inner.reason)
.field("wasm_trace", &self.inner.wasm_trace)
.field("native_trace", &self.inner.native_trace)
.finish()
@@ -159,7 +204,7 @@ impl fmt::Debug for Trap {
impl fmt::Display for Trap {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{}", self.inner.message)?;
write!(f, "{}", self.inner.reason)?;
let trace = self.trace();
if trace.is_empty() {
return Ok(());

View File

@@ -123,7 +123,7 @@ impl WastContext {
fn module(&mut self, instance_name: Option<&str>, module: &[u8]) -> Result<()> {
let instance = match self.instantiate(module)? {
Outcome::Ok(i) => i,
Outcome::Trap(e) => bail!("instantiation failed with: {}", e.message()),
Outcome::Trap(e) => bail!("instantiation failed: {}", e.message()),
};
if let Some(name) = instance_name {
self.linker.instance(name, &instance)?;

View File

@@ -142,13 +142,24 @@ impl RunCommand {
{
Ok(()) => (),
Err(e) => {
// If the program exited because of a trap, return an error code
// to the outside environment indicating a more severe problem
// than a simple failure.
if e.is::<Trap>() {
// If the program exited because of a non-zero exit status, print
// a message and exit.
if let Some(trap) = e.downcast_ref::<Trap>() {
// Print the error message in the usual way.
eprintln!("Error: {:?}", e);
if let Some(status) = trap.i32_exit_status() {
// On Windows, exit status 3 indicates an abort (see below),
// so return 1 indicating a non-zero status to avoid ambiguity.
if cfg!(windows) && status >= 3 {
process::exit(1);
}
process::exit(status);
}
// If the program exited because of a trap, return an error code
// to the outside environment indicating a more severe problem
// than a simple failure.
if cfg!(unix) {
// On Unix, return the error code of an abort.
process::exit(128 + libc::SIGABRT);

View File

@@ -182,3 +182,77 @@ fn timeout_in_invoke() -> Result<()> {
);
Ok(())
}
// Exit with a valid non-zero exit code, snapshot0 edition.
#[test]
fn exit2_wasi_snapshot0() -> Result<()> {
let wasm = build_wasm("tests/wasm/exit2_wasi_snapshot0.wat")?;
let output = run_wasmtime_for_output(&[wasm.path().to_str().unwrap(), "--disable-cache"])?;
assert_eq!(output.status.code().unwrap(), 2);
Ok(())
}
// Exit with a valid non-zero exit code, snapshot1 edition.
#[test]
fn exit2_wasi_snapshot1() -> Result<()> {
let wasm = build_wasm("tests/wasm/exit2_wasi_snapshot1.wat")?;
let output = run_wasmtime_for_output(&[wasm.path().to_str().unwrap(), "--disable-cache"])?;
assert_eq!(output.status.code().unwrap(), 2);
Ok(())
}
// Exit with a valid non-zero exit code, snapshot0 edition.
#[test]
fn exit125_wasi_snapshot0() -> Result<()> {
let wasm = build_wasm("tests/wasm/exit125_wasi_snapshot0.wat")?;
let output = run_wasmtime_for_output(&[wasm.path().to_str().unwrap(), "--disable-cache"])?;
if cfg!(windows) {
assert_eq!(output.status.code().unwrap(), 1);
} else {
assert_eq!(output.status.code().unwrap(), 125);
}
Ok(())
}
// Exit with a valid non-zero exit code, snapshot1 edition.
#[test]
fn exit125_wasi_snapshot1() -> Result<()> {
let wasm = build_wasm("tests/wasm/exit125_wasi_snapshot1.wat")?;
let output = run_wasmtime_for_output(&[wasm.path().to_str().unwrap(), "--disable-cache"])?;
if cfg!(windows) {
assert_eq!(output.status.code().unwrap(), 1);
} else {
assert_eq!(output.status.code().unwrap(), 125);
}
Ok(())
}
// Exit with an invalid non-zero exit code, snapshot0 edition.
#[test]
fn exit126_wasi_snapshot0() -> Result<()> {
let wasm = build_wasm("tests/wasm/exit126_wasi_snapshot0.wat")?;
let output = run_wasmtime_for_output(&[wasm.path().to_str().unwrap(), "--disable-cache"])?;
if cfg!(windows) {
assert_eq!(output.status.code().unwrap(), 3);
} else {
assert_eq!(output.status.code().unwrap(), 128 + libc::SIGABRT);
}
assert!(output.stdout.is_empty());
assert!(String::from_utf8_lossy(&output.stderr).contains("invalid exit status"));
Ok(())
}
// Exit with an invalid non-zero exit code, snapshot1 edition.
#[test]
fn exit126_wasi_snapshot1() -> Result<()> {
let wasm = build_wasm("tests/wasm/exit126_wasi_snapshot1.wat")?;
let output = run_wasmtime_for_output(&[wasm.path().to_str().unwrap(), "--disable-cache"])?;
if cfg!(windows) {
assert_eq!(output.status.code().unwrap(), 3);
} else {
assert_eq!(output.status.code().unwrap(), 128 + libc::SIGABRT);
}
assert!(output.stdout.is_empty());
assert!(String::from_utf8_lossy(&output.stderr).contains("invalid exit status"));
Ok(())
}

View File

@@ -182,6 +182,7 @@ fn trap_smoke() -> Result<()> {
let f = Func::wrap(&store, || -> Result<(), Trap> { Err(Trap::new("test")) });
let err = f.call(&[]).unwrap_err().downcast::<Trap>()?;
assert_eq!(err.message(), "test");
assert!(err.i32_exit_status().is_none());
Ok(())
}

View File

@@ -30,7 +30,7 @@ fn loops_interruptable() -> anyhow::Result<()> {
let iloop = instance.get_func("loop").unwrap().get0::<()>()?;
store.interrupt_handle()?.interrupt();
let trap = iloop().unwrap_err();
assert!(trap.message().contains("wasm trap: interrupt"));
assert!(trap.to_string().contains("wasm trap: interrupt"));
Ok(())
}
@@ -44,9 +44,9 @@ fn functions_interruptable() -> anyhow::Result<()> {
store.interrupt_handle()?.interrupt();
let trap = iloop().unwrap_err();
assert!(
trap.message().contains("wasm trap: interrupt"),
trap.to_string().contains("wasm trap: interrupt"),
"{}",
trap.message()
trap.to_string()
);
Ok(())
}
@@ -91,9 +91,9 @@ fn loop_interrupt_from_afar() -> anyhow::Result<()> {
let trap = iloop().unwrap_err();
thread.join().unwrap();
assert!(
trap.message().contains("wasm trap: interrupt"),
trap.to_string().contains("wasm trap: interrupt"),
"bad message: {}",
trap.message()
trap.to_string()
);
Ok(())
}
@@ -127,9 +127,9 @@ fn function_interrupt_from_afar() -> anyhow::Result<()> {
let trap = iloop().unwrap_err();
thread.join().unwrap();
assert!(
trap.message().contains("wasm trap: interrupt"),
trap.to_string().contains("wasm trap: interrupt"),
"bad message: {}",
trap.message()
trap.to_string()
);
Ok(())
}

View File

@@ -30,9 +30,9 @@ fn host_always_has_some_stack() -> anyhow::Result<()> {
// has been exhausted.
let trap = foo().unwrap_err();
assert!(
trap.message().contains("call stack exhausted"),
trap.to_string().contains("call stack exhausted"),
"{}",
trap.message()
trap.to_string()
);
// Additionally, however, and this is the crucial test, make sure that the

View File

@@ -0,0 +1,8 @@
(module
(import "wasi_unstable" "proc_exit"
(func $__wasi_proc_exit (param i32)))
(func $_start
(call $__wasi_proc_exit (i32.const 125))
)
(export "_start" (func $_start))
)

View File

@@ -0,0 +1,8 @@
(module
(import "wasi_snapshot_preview1" "proc_exit"
(func $__wasi_proc_exit (param i32)))
(func $_start
(call $__wasi_proc_exit (i32.const 125))
)
(export "_start" (func $_start))
)

View File

@@ -0,0 +1,8 @@
(module
(import "wasi_unstable" "proc_exit"
(func $__wasi_proc_exit (param i32)))
(func $_start
(call $__wasi_proc_exit (i32.const 126))
)
(export "_start" (func $_start))
)

View File

@@ -0,0 +1,8 @@
(module
(import "wasi_snapshot_preview1" "proc_exit"
(func $__wasi_proc_exit (param i32)))
(func $_start
(call $__wasi_proc_exit (i32.const 126))
)
(export "_start" (func $_start))
)

View File

@@ -0,0 +1,8 @@
(module
(import "wasi_unstable" "proc_exit"
(func $__wasi_proc_exit (param i32)))
(func $_start
(call $__wasi_proc_exit (i32.const 2))
)
(export "_start" (func $_start))
)

View File

@@ -0,0 +1,8 @@
(module
(import "wasi_snapshot_preview1" "proc_exit"
(func $__wasi_proc_exit (param i32)))
(func $_start
(call $__wasi_proc_exit (i32.const 2))
)
(export "_start" (func $_start))
)