Remove unsafety from Trap API (#779)

* Remove unsafety from `Trap` API

This commit removes the `unsafe impl Send` for `Trap` by removing the
internal `HostRef` and leaving `HostRef` entirely as an implementation
detail of the C API.

cc #708

* Run rustfmt

Cargo.lock

@@ -1971,7 +1971,6 @@ dependencies = [
  "rayon",
  "region",
  "target-lexicon",
- "thiserror",
  "wasi-common",
  "wasmparser 0.45.1",
  "wasmtime-environ",

crates/api/Cargo.toml

@@ -19,7 +19,6 @@ wasmtime-jit = { path = "../jit" }
 wasmparser = { version = "0.45.1", default-features = false }
 target-lexicon = { version = "0.9.0", default-features = false }
 anyhow = "1.0.19"
-thiserror = "1.0.4"
 region = "2.0.0"
 
 [dev-dependencies]

crates/api/src/callable.rs

@@ -1,7 +1,6 @@
-use crate::r#ref::HostRef;
 use crate::runtime::Store;
 use crate::trampoline::{generate_func_export, take_api_trap};
-use crate::trap::{Trap, TrapInfo};
+use crate::trap::Trap;
 use crate::types::FuncType;
 use crate::values::Val;
 use std::rc::Rc;
@@ -158,9 +157,9 @@ impl WrappedCallable for WasmtimeFn {
                 values_vec.as_mut_ptr() as *mut u8,
             )
         } {
-            let trap = take_api_trap()
-                .unwrap_or_else(|| HostRef::new(TrapInfo::new(format!("call error: {}", message))));
-            return Err(trap.into());
+            let trap =
+                take_api_trap().unwrap_or_else(|| Trap::new(format!("call error: {}", message)));
+            return Err(trap);
         }
 
         // Load the return values out of `values_vec`.

crates/api/src/instance.rs

@@ -45,7 +45,7 @@ pub fn instantiate_in_context(
     )
     .map_err(|e| -> Error {
         if let Some(trap) = take_api_trap() {
-            Trap::from(trap).into()
+            trap.into()
         } else if let SetupError::Instantiate(InstantiationError::StartTrap(msg)) = e {
             Trap::new(msg).into()
         } else {

crates/api/src/lib.rs

@@ -28,6 +28,6 @@ pub use crate::instance::Instance;
 pub use crate::module::Module;
 pub use crate::r#ref::{AnyRef, HostInfo, HostRef};
 pub use crate::runtime::{Config, Engine, OptLevel, Store, Strategy};
-pub use crate::trap::{FrameInfo, Trap, TrapInfo};
+pub use crate::trap::{FrameInfo, Trap};
 pub use crate::types::*;
 pub use crate::values::*;

crates/api/src/trampoline/func.rs

@@ -95,7 +95,6 @@ unsafe extern "C" fn stub_fn(vmctx: *mut VMContext, call_id: u32, values_vec: *m
             0
         }
         Err(trap) => {
-            let trap = trap.trap_info_unchecked();
             record_api_trap(trap);
             1
         }

crates/api/src/trampoline/trap.rs

@@ -1,7 +1,6 @@
 use std::cell::Cell;
 
-use crate::r#ref::HostRef;
-use crate::TrapInfo;
+use crate::Trap;
 use wasmtime_environ::ir::{SourceLoc, TrapCode};
 use wasmtime_environ::TrapInformation;
 use wasmtime_jit::trampoline::binemit;
@@ -10,10 +9,10 @@ use wasmtime_jit::trampoline::binemit;
 pub const API_TRAP_CODE: TrapCode = TrapCode::User(13);
 
 thread_local! {
-    static RECORDED_API_TRAP: Cell<Option<HostRef<TrapInfo>>> = Cell::new(None);
+    static RECORDED_API_TRAP: Cell<Option<Trap>> = Cell::new(None);
 }
 
-pub fn record_api_trap(trap: HostRef<TrapInfo>) {
+pub fn record_api_trap(trap: Trap) {
     RECORDED_API_TRAP.with(|data| {
         let trap = Cell::new(Some(trap));
         data.swap(&trap);
@@ -24,7 +23,7 @@ pub fn record_api_trap(trap: HostRef<TrapInfo>) {
     });
 }
 
-pub fn take_api_trap() -> Option<HostRef<TrapInfo>> {
+pub fn take_api_trap() -> Option<Trap> {
     RECORDED_API_TRAP.with(|data| data.take())
 }
 

crates/api/src/trap.rs

@@ -1,8 +1,65 @@
 use crate::instance::Instance;
-use crate::r#ref::HostRef;
 use std::fmt;
-use std::sync::{Arc, Mutex};
-use thiserror::Error;
+use std::sync::Arc;
+
+/// A struct representing an aborted instruction execution, with a message
+/// indicating the cause.
+#[derive(Clone)]
+pub struct Trap {
+    inner: Arc<TrapInner>,
+}
+
+struct TrapInner {
+    message: String,
+    trace: Vec<FrameInfo>,
+}
+
+fn _assert_trap_is_sync_and_send(t: &Trap) -> (&dyn Sync, &dyn Send) {
+    (t, t)
+}
+
+impl Trap {
+    /// Creates a new `Trap` with `message`.
+    /// # Example
+    /// ```
+    /// let trap = wasmtime::Trap::new("unexpected error");
+    /// assert_eq!("unexpected error", trap.message());
+    /// ```
+    pub fn new<I: Into<String>>(message: I) -> Self {
+        Trap {
+            inner: Arc::new(TrapInner {
+                message: message.into(),
+                trace: Vec::new(),
+            }),
+        }
+    }
+
+    /// Returns a reference the `message` stored in `Trap`.
+    pub fn message(&self) -> &str {
+        &self.inner.message
+    }
+
+    pub fn trace(&self) -> &[FrameInfo] {
+        &self.inner.trace
+    }
+}
+
+impl fmt::Debug for Trap {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("Trap")
+            .field("message", &self.inner.message)
+            .field("trace", &self.inner.trace)
+            .finish()
+    }
+}
+
+impl fmt::Display for Trap {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.inner.message.fmt(f)
+    }
+}
+
+impl std::error::Error for Trap {}
 
 #[derive(Debug)]
 pub struct FrameInfo;
@@ -24,94 +81,3 @@ impl FrameInfo {
         unimplemented!("FrameInfo::module_offset");
     }
 }
-
-#[derive(Debug)]
-pub struct TrapInfo {
-    message: String,
-    trace: Vec<FrameInfo>,
-}
-
-impl TrapInfo {
-    pub fn new<I: Into<String>>(message: I) -> Self {
-        Self {
-            message: message.into(),
-            trace: vec![],
-        }
-    }
-
-    /// Returns a reference the `message` stored in `Trap`.
-    pub fn message(&self) -> &str {
-        &self.message
-    }
-
-    pub fn origin(&self) -> Option<&FrameInfo> {
-        self.trace.first()
-    }
-
-    pub fn trace(&self) -> &[FrameInfo] {
-        &self.trace
-    }
-}
-
-/// A struct to hold unsafe TrapInfo host reference, designed
-/// to be Send-able. The only access for it provided via the
-/// Trap::trap_info_unchecked() method.
-struct UnsafeTrapInfo(HostRef<TrapInfo>);
-
-impl UnsafeTrapInfo {
-    fn trap_info(&self) -> HostRef<TrapInfo> {
-        self.0.clone()
-    }
-}
-
-unsafe impl Send for UnsafeTrapInfo {}
-
-impl fmt::Debug for UnsafeTrapInfo {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(f, "UnsafeTrapInfo")
-    }
-}
-
-/// A struct representing an aborted instruction execution, with a message
-/// indicating the cause.
-#[derive(Error, Debug, Clone)]
-#[error("Wasm trap: {message}")]
-pub struct Trap {
-    message: String,
-    info: Arc<Mutex<UnsafeTrapInfo>>,
-}
-
-fn _assert_trap_is_sync_and_send(t: &Trap) -> (&dyn Sync, &dyn Send) {
-    (t, t)
-}
-
-impl Trap {
-    /// Creates a new `Trap` with `message`.
-    /// # Example
-    /// ```
-    /// let trap = wasmtime::Trap::new("unexpected error");
-    /// assert_eq!("unexpected error", trap.message());
-    /// ```
-    pub fn new<I: Into<String>>(message: I) -> Self {
-        Trap::from(HostRef::new(TrapInfo::new(message)))
-    }
-
-    /// Returns a reference the `message` stored in `Trap`.
-    pub fn message(&self) -> &str {
-        &self.message
-    }
-
-    /// Returns inner TrapInfo assotiated with the Trap.
-    /// The method is unsafe: obtained TrapInfo is not thread safe.
-    pub(crate) unsafe fn trap_info_unchecked(&self) -> HostRef<TrapInfo> {
-        self.info.lock().unwrap().trap_info()
-    }
-}
-
-impl From<HostRef<TrapInfo>> for Trap {
-    fn from(trap_info: HostRef<TrapInfo>) -> Self {
-        let message = trap_info.borrow().message().to_string();
-        let info = Arc::new(Mutex::new(UnsafeTrapInfo(trap_info)));
-        Trap { message, info }
-    }
-}

crates/api/src/wasm.rs

@@ -8,7 +8,7 @@
 use super::{
     AnyRef, Callable, Engine, ExportType, Extern, ExternType, Func, FuncType, Global, GlobalType,
     HostInfo, HostRef, ImportType, Instance, Limits, Memory, MemoryType, Module, Store, Table,
-    TableType, Trap, TrapInfo, Val, ValType,
+    TableType, Trap, Val, ValType,
 };
 use std::rc::Rc;
 use std::{mem, ptr, slice};
@@ -317,7 +317,7 @@ pub type wasm_message_t = wasm_name_t;
 #[repr(C)]
 #[derive(Clone)]
 pub struct wasm_trap_t {
-    trap_info: HostRef<TrapInfo>,
+    trap: HostRef<Trap>,
 }
 #[repr(C)]
 #[derive(Clone)]
@@ -469,8 +469,9 @@ pub unsafe extern "C" fn wasm_func_call(
             ptr::null_mut()
         }
         Err(trap) => {
-            let trap_info = trap.trap_info_unchecked();
-            let trap = Box::new(wasm_trap_t { trap_info });
+            let trap = Box::new(wasm_trap_t {
+                trap: HostRef::new(trap),
+            });
             Box::into_raw(trap)
         }
     }
@@ -550,8 +551,7 @@ impl Callable for wasm_func_callback_t {
         let out = unsafe { func(params.as_ptr(), out_results.as_mut_ptr()) };
         if !out.is_null() {
             let trap: Box<wasm_trap_t> = unsafe { Box::from_raw(out) };
-            let trap_info: HostRef<TrapInfo> = (*trap).into();
-            return Err(Trap::from(trap_info));
+            return Err(trap.trap.borrow().clone());
         }
         for i in 0..results.len() {
             results[i] = out_results[i].val();
@@ -560,12 +560,6 @@ impl Callable for wasm_func_callback_t {
     }
 }
 
-impl Into<HostRef<TrapInfo>> for wasm_trap_t {
-    fn into(self) -> HostRef<TrapInfo> {
-        self.trap_info
-    }
-}
-
 struct CallbackWithEnv {
     callback: wasm_func_callback_with_env_t,
     env: *mut std::ffi::c_void,
@@ -583,8 +577,7 @@ impl Callable for CallbackWithEnv {
         let out = unsafe { func(self.env, params.as_ptr(), out_results.as_mut_ptr()) };
         if !out.is_null() {
             let trap: Box<wasm_trap_t> = unsafe { Box::from_raw(out) };
-            let trap_info: HostRef<TrapInfo> = (*trap).into();
-            return Err(Trap::from(trap_info));
+            return Err(trap.trap.borrow().clone());
         }
         for i in 0..results.len() {
             results[i] = out_results[i].val();
@@ -682,11 +675,13 @@ pub unsafe extern "C" fn wasm_instance_new(
         }
         Err(trap) => {
             if !result.is_null() {
-                let trap_info = match trap.downcast::<Trap>() {
-                    Ok(trap) => trap.trap_info_unchecked(),
-                    Err(_) => HostRef::new(TrapInfo::new("instance error".to_string())),
+                let trap = match trap.downcast::<Trap>() {
+                    Ok(trap) => trap,
+                    Err(e) => Trap::new(format!("{:?}", e)),
                 };
-                let trap = Box::new(wasm_trap_t { trap_info });
+                let trap = Box::new(wasm_trap_t {
+                    trap: HostRef::new(trap),
+                });
                 (*result) = Box::into_raw(trap);
             }
             ptr::null_mut()
@@ -927,9 +922,9 @@ pub unsafe extern "C" fn wasm_trap_new(
     if message[message.len() - 1] != 0 {
         panic!("wasm_trap_new message stringz expected");
     }
-    let message = String::from_utf8_lossy(message).to_string();
+    let message = String::from_utf8_lossy(message);
     let trap = Box::new(wasm_trap_t {
-        trap_info: HostRef::new(TrapInfo::new(message)),
+        trap: HostRef::new(Trap::new(message)),
     });
     Box::into_raw(trap)
 }
@@ -937,7 +932,7 @@ pub unsafe extern "C" fn wasm_trap_new(
 #[no_mangle]
 pub unsafe extern "C" fn wasm_trap_message(trap: *const wasm_trap_t, out: *mut wasm_message_t) {
     let mut buffer = Vec::new();
-    buffer.extend_from_slice((*trap).trap_info.borrow().message().as_bytes());
+    buffer.extend_from_slice((*trap).trap.borrow().message().as_bytes());
     buffer.reserve_exact(1);
     buffer.push(0);
     (*out).set_buffer(buffer);