From b0ed09d3b6ae512c8dcde299b4f10719fda5c030 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Tue, 10 Mar 2020 12:52:45 -0700 Subject: [PATCH 1/8] Exit with a more severe error code if the program traps. Previously, the wasmtime CLI would return with a regular failure error code, such as 1 on Unix. However, a program trap indicates a bug in the program, which can be useful to distinguish from a simple error status. Check for the trap case, and return an appropriate OS-specific exit status. --- src/commands/run.rs | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/src/commands/run.rs b/src/commands/run.rs index 7407f83c49f1..894c7b4a380b 100644 --- a/src/commands/run.rs +++ b/src/commands/run.rs @@ -6,10 +6,11 @@ use std::{ ffi::{OsStr, OsString}, fs::File, path::{Component, Path, PathBuf}, + process, }; use structopt::{clap::AppSettings, StructOpt}; use wasi_common::preopen_dir; -use wasmtime::{Engine, Instance, Module, Store}; +use wasmtime::{Engine, Instance, Module, Store, Trap}; use wasmtime_interface_types::ModuleData; use wasmtime_wasi::{old::snapshot_0::Wasi as WasiSnapshot0, Wasi}; @@ -113,8 +114,35 @@ impl RunCommand { } // Load the main wasm module. - self.handle_module(&store, &module_registry) - .with_context(|| format!("failed to run main module `{}`", self.module.display()))?; + match self + .handle_module(&store, &module_registry) + .with_context(|| format!("failed to run main module `{}`", self.module.display())) + { + 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 let Some(source) = e.source() { + if let Some(source) = source.source() { + if source.is::() { + // Print the error message in the usual way. + eprintln!("Error: {:?}", e); + + // On Unix, return the error code of an abort. + #[cfg(unix)] + process::exit(128 + libc::SIGABRT); + + // On Windows, return 3. + // https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/abort?view=vs-2019 + #[cfg(windows)] + process::exit(3); + } + } + } + return Err(e); + } + } Ok(()) } From 2f7819f2aebe320a03a908528b3a2007303b102c Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Tue, 10 Mar 2020 16:23:02 -0700 Subject: [PATCH 2/8] Use a loop to iterate over the error causes to find Traps. --- src/commands/run.rs | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/commands/run.rs b/src/commands/run.rs index 894c7b4a380b..1c80fb1e75a0 100644 --- a/src/commands/run.rs +++ b/src/commands/run.rs @@ -123,22 +123,22 @@ impl RunCommand { // 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 let Some(source) = e.source() { - if let Some(source) = source.source() { - if source.is::() { - // Print the error message in the usual way. - eprintln!("Error: {:?}", e); - - // On Unix, return the error code of an abort. - #[cfg(unix)] - process::exit(128 + libc::SIGABRT); - - // On Windows, return 3. - // https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/abort?view=vs-2019 - #[cfg(windows)] - process::exit(3); - } + let mut err = e.source(); + while let Some(source) = err { + if source.is::() { + // Print the error message in the usual way. + eprintln!("Error: {:?}", e); + + // On Unix, return the error code of an abort. + #[cfg(unix)] + process::exit(128 + libc::SIGABRT); + + // On Windows, return 3. + // https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/abort?view=vs-2019 + #[cfg(windows)] + process::exit(3); } + err = source.source(); } return Err(e); } From 97f24f61323cfa206daf66e0b202ee7829295429 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Tue, 10 Mar 2020 16:47:27 -0700 Subject: [PATCH 3/8] Use anyhow's `chain()` iterator. --- src/commands/run.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/commands/run.rs b/src/commands/run.rs index 1c80fb1e75a0..d890afc1c192 100644 --- a/src/commands/run.rs +++ b/src/commands/run.rs @@ -123,9 +123,8 @@ impl RunCommand { // 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. - let mut err = e.source(); - while let Some(source) = err { - if source.is::() { + for cause in e.chain() { + if cause.is::() { // Print the error message in the usual way. eprintln!("Error: {:?}", e); @@ -138,7 +137,6 @@ impl RunCommand { #[cfg(windows)] process::exit(3); } - err = source.source(); } return Err(e); } From 4090c432bb17529decb89e5867d4576424c6d8e8 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Tue, 10 Mar 2020 16:51:05 -0700 Subject: [PATCH 4/8] For completeness, handle non-Unix and non-Windows platforms too. --- src/commands/run.rs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/commands/run.rs b/src/commands/run.rs index d890afc1c192..7ab02e8578ef 100644 --- a/src/commands/run.rs +++ b/src/commands/run.rs @@ -128,14 +128,17 @@ impl RunCommand { // Print the error message in the usual way. eprintln!("Error: {:?}", e); - // On Unix, return the error code of an abort. - #[cfg(unix)] - process::exit(128 + libc::SIGABRT); - - // On Windows, return 3. - // https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/abort?view=vs-2019 - #[cfg(windows)] - process::exit(3); + if cfg!(unix) { + // On Unix, return the error code of an abort. + process::exit(128 + libc::SIGABRT); + } else if cfg!(windows) { + // On Windows, return 3. + // https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/abort?view=vs-2019 + process::exit(3); + } else { + // Otherwise just exit with a normal error. + break; + } } } return Err(e); From 51c9aaefb41c1b41c0cb894a055838040ad1801f Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Wed, 11 Mar 2020 06:49:54 -0700 Subject: [PATCH 5/8] Add a CLI test for a trapping program. --- tests/cli_tests.rs | 37 ++++++++++++++++++++++++++++++++++--- tests/wasm/unreachable.wat | 5 +++++ 2 files changed, 39 insertions(+), 3 deletions(-) create mode 100644 tests/wasm/unreachable.wat diff --git a/tests/cli_tests.rs b/tests/cli_tests.rs index 3387158d5632..48e901534338 100644 --- a/tests/cli_tests.rs +++ b/tests/cli_tests.rs @@ -1,15 +1,22 @@ use anyhow::{bail, Result}; use std::io::Write; use std::path::Path; -use std::process::Command; +use std::process::{Command, Output}; use tempfile::NamedTempFile; -fn run_wasmtime(args: &[&str]) -> Result { +// Run the wasmtime CLI with the provided args and return the `Output`. +fn run_wasmtime_for_output(args: &[&str]) -> Result { let mut me = std::env::current_exe()?; me.pop(); // chop off the file name me.pop(); // chop off `deps` me.push("wasmtime"); - let output = Command::new(&me).args(args).output()?; + Command::new(&me).args(args).output().map_err(Into::into) +} + +// Run the wasmtime CLI with the provided args and, if it succeeds, return +// the standard output in a `String`. +fn run_wasmtime(args: &[&str]) -> Result { + let output = run_wasmtime_for_output(args)?; if !output.status.success() { bail!( "Failed to execute wasmtime with: {:?}\n{}", @@ -74,3 +81,27 @@ fn run_wasmtime_simple_wat() -> Result<()> { ])?; Ok(()) } + +// Running a wat that traps. +#[test] +fn run_wasmtime_unreachable_wat() -> Result<()> { + let wasm = build_wasm("tests/wasm/unreachable.wat")?; + let output = run_wasmtime_for_output(&[wasm.path().to_str().unwrap(), "--disable-cache"])?; + + assert!(output.stdout.is_empty()); + assert!(!output.stderr.is_empty()); + assert!(!output.status.success()); + + let code = output + .status + .code() + .expect("wasmtime process should exit normally"); + + // Test for the specific error code Wasmtime uses to indicate a trap return. + #[cfg(unix)] + assert_eq!(code, 128 + libc::SIGABRT); + #[cfg(windows)] + assert_eq!(code, 4); + + Ok(()) +} diff --git a/tests/wasm/unreachable.wat b/tests/wasm/unreachable.wat new file mode 100644 index 000000000000..0a81de37f178 --- /dev/null +++ b/tests/wasm/unreachable.wat @@ -0,0 +1,5 @@ +(module + (func (export "_start") + unreachable + ) +) From e10a1f2d89c6e28e645e78359f758e9548d34deb Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Wed, 11 Mar 2020 06:55:28 -0700 Subject: [PATCH 6/8] Replace a manual `.cause` loop with a `.is` call. --- src/commands/run.rs | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/src/commands/run.rs b/src/commands/run.rs index 7ab02e8578ef..c0e9ca13c0fd 100644 --- a/src/commands/run.rs +++ b/src/commands/run.rs @@ -123,22 +123,17 @@ impl RunCommand { // 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. - for cause in e.chain() { - if cause.is::() { - // Print the error message in the usual way. - eprintln!("Error: {:?}", e); - - if cfg!(unix) { - // On Unix, return the error code of an abort. - process::exit(128 + libc::SIGABRT); - } else if cfg!(windows) { - // On Windows, return 3. - // https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/abort?view=vs-2019 - process::exit(3); - } else { - // Otherwise just exit with a normal error. - break; - } + if e.is::() { + // Print the error message in the usual way. + eprintln!("Error: {:?}", e); + + if cfg!(unix) { + // On Unix, return the error code of an abort. + process::exit(128 + libc::SIGABRT); + } else if cfg!(windows) { + // On Windows, return 3. + // https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/abort?view=vs-2019 + process::exit(3); } } return Err(e); From 1ea36c17919b963221e6e5d4f99e068287ec1695 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Wed, 11 Mar 2020 07:23:11 -0700 Subject: [PATCH 7/8] Correct the expected exit status on Windows. --- tests/cli_tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/cli_tests.rs b/tests/cli_tests.rs index 48e901534338..4ae9a064c11f 100644 --- a/tests/cli_tests.rs +++ b/tests/cli_tests.rs @@ -101,7 +101,7 @@ fn run_wasmtime_unreachable_wat() -> Result<()> { #[cfg(unix)] assert_eq!(code, 128 + libc::SIGABRT); #[cfg(windows)] - assert_eq!(code, 4); + assert_eq!(code, 3); Ok(()) } From ba43b95143dfd81eb1ba29c44caaaf2ac8c882fe Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Wed, 11 Mar 2020 09:50:56 -0700 Subject: [PATCH 8/8] Use assert_eq/assert_ne so that if these fail, it prints the output. --- tests/cli_tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/cli_tests.rs b/tests/cli_tests.rs index 4ae9a064c11f..d13970504fd0 100644 --- a/tests/cli_tests.rs +++ b/tests/cli_tests.rs @@ -88,8 +88,8 @@ fn run_wasmtime_unreachable_wat() -> Result<()> { let wasm = build_wasm("tests/wasm/unreachable.wat")?; let output = run_wasmtime_for_output(&[wasm.path().to_str().unwrap(), "--disable-cache"])?; - assert!(output.stdout.is_empty()); - assert!(!output.stderr.is_empty()); + assert_ne!(output.stderr, b""); + assert_eq!(output.stdout, b""); assert!(!output.status.success()); let code = output