Don't try to handle non-wasmtime segfaults (#1577)

This commit fixes an issue in Wasmtime where Wasmtime would accidentally
"handle" non-wasm segfaults while executing host imports of wasm
modules. If a host import segfaulted then Wasmtime would recognize that
wasm code is on the stack, so it'd longjmp out of the wasm code. This
papers over real bugs though in host code and erroneously classified
segfaults as wasm traps.

The fix here was to add a check to our wasm signal handler for if the
faulting address falls in JIT code itself. Actually threading through
all the right information for that check to happen is a bit tricky,
though, so this involved some refactoring:

* A closure parameter to `catch_traps` was added. This closure is
  responsible for classifying addresses as whether or not they fall in
  JIT code. Anything returning `false` means that the trap won't get
  handled and we'll forward to the next signal handler.

* To avoid passing tons of context all over the place, the start
  function is now no longer automatically invoked by `InstanceHandle`.
  This avoids the need for passing all sorts of trap-handling contextual
  information like the maximum stack size and "is this a jit address"
  closure. Instead creators of `InstanceHandle` (like wasmtime) are now
  responsible for invoking the start function.

* To avoid excessive use of `transmute` with lifetimes since the
  traphandler state now has a lifetime the per-instance custom signal
  handler is now replaced with a per-store custom signal handler. I'm
  not entirely certain the purpose of the custom signal handler, though,
  so I'd look for feedback on this part.

A new test has been added which ensures that if a host function
segfaults we don't accidentally try to handle it, and instead we
correctly report the segfault.

crates/api/src/func.rs

@@ -178,7 +178,6 @@ macro_rules! getters {
             // of the closure. Pass the export in so that we can call it.
             let instance = self.instance.clone();
             let export = self.export.clone();
-            let max_wasm_stack = self.store().engine().config().max_wasm_stack;
 
             // ... and then once we've passed the typechecks we can hand out our
             // object since our `transmute` below should be safe!
@@ -194,13 +193,9 @@ macro_rules! getters {
                     >(export.address);
                     let mut ret = None;
                     $(let $args = $args.into_abi();)*
-                    wasmtime_runtime::catch_traps(export.vmctx, max_wasm_stack, || {
+                    catch_traps(export.vmctx, &instance.store, || {
                         ret = Some(fnptr(export.vmctx, ptr::null_mut(), $($args,)*));
-                    }).map_err(Trap::from_jit)?;
-
-                    // We're holding this handle just to ensure that the instance stays
-                    // live while we call into it.
-                    drop(&instance);
+                    })?;
 
                     Ok(ret.unwrap())
                 }
@@ -562,22 +557,14 @@ impl Func {
         }
 
         // Call the trampoline.
-        if let Err(error) = unsafe {
-            wasmtime_runtime::catch_traps(
+        catch_traps(self.export.vmctx, &self.instance.store, || unsafe {
+            (self.trampoline)(
                 self.export.vmctx,
-                self.instance.store.engine().config().max_wasm_stack,
-                || {
-                    (self.trampoline)(
-                        self.export.vmctx,
-                        ptr::null_mut(),
-                        self.export.address,
-                        values_vec.as_mut_ptr(),
-                    )
-                },
+                ptr::null_mut(),
+                self.export.address,
+                values_vec.as_mut_ptr(),
             )
-        } {
-            return Err(Trap::from_jit(error).into());
-        }
+        })?;
 
         // Load the return values out of `values_vec`.
         let mut results = Vec::with_capacity(my_ty.results().len());
@@ -746,6 +733,24 @@ impl fmt::Debug for Func {
     }
 }
 
+pub(crate) fn catch_traps(
+    vmctx: *mut VMContext,
+    store: &Store,
+    closure: impl FnMut(),
+) -> Result<(), Trap> {
+    let signalhandler = store.signal_handler();
+    unsafe {
+        wasmtime_runtime::catch_traps(
+            vmctx,
+            store.engine().config().max_wasm_stack,
+            |addr| store.compiler().is_in_jit_code(addr),
+            signalhandler.as_deref(),
+            closure,
+        )
+        .map_err(Trap::from_jit)
+    }
+}
+
 /// A trait implemented for types which can be arguments to closures passed to
 /// [`Func::wrap`] and friends.
 ///

crates/api/src/instance.rs

@@ -2,8 +2,10 @@ use crate::trampoline::StoreInstanceHandle;
 use crate::{Export, Extern, Func, Global, Memory, Module, Store, Table, Trap};
 use anyhow::{bail, Error, Result};
 use std::any::Any;
+use std::mem;
+use wasmtime_environ::EntityIndex;
 use wasmtime_jit::{CompiledModule, Resolver};
-use wasmtime_runtime::{InstantiationError, SignatureRegistry};
+use wasmtime_runtime::{InstantiationError, SignatureRegistry, VMContext, VMFunctionBody};
 
 struct SimpleResolver<'a> {
     imports: &'a [Extern],
@@ -45,7 +47,6 @@ fn instantiate(
         instance
             .initialize(
                 config.validating_config.operator_config.enable_bulk_memory,
-                config.max_wasm_stack,
                 &compiled_module.data_initializers(),
             )
             .map_err(|e| -> Error {
@@ -56,6 +57,23 @@ fn instantiate(
                     other => other.into(),
                 }
             })?;
+
+        // If a start function is present, now that we've got our compiled
+        // instance we can invoke it. Make sure we use all the trap-handling
+        // configuration in `store` as well.
+        if let Some(start) = instance.module().start_func {
+            let f = match instance.lookup_by_declaration(&EntityIndex::Function(start)) {
+                wasmtime_runtime::Export::Function(f) => f,
+                _ => unreachable!(), // valid modules shouldn't hit this
+            };
+            super::func::catch_traps(instance.vmctx_ptr(), store, || {
+                mem::transmute::<
+                    *const VMFunctionBody,
+                    unsafe extern "C" fn(*mut VMContext, *mut VMContext),
+                >(f.address)(f.vmctx, instance.vmctx_ptr())
+            })?;
+        }
+
         Ok(instance)
     }
 }

crates/api/src/runtime.rs

@@ -12,7 +12,9 @@ use wasmtime_environ::settings::{self, Configurable};
 use wasmtime_environ::{CacheConfig, Tunables};
 use wasmtime_jit::{native, CompilationStrategy, Compiler};
 use wasmtime_profiling::{JitDumpAgent, NullProfilerAgent, ProfilingAgent, VTuneAgent};
-use wasmtime_runtime::{debug_builtins, InstanceHandle, RuntimeMemoryCreator, VMInterrupts};
+use wasmtime_runtime::{
+    debug_builtins, InstanceHandle, RuntimeMemoryCreator, SignalHandler, VMInterrupts,
+};
 
 // Runtime Environment
 
@@ -557,6 +559,7 @@ pub(crate) struct StoreInner {
     engine: Engine,
     compiler: RefCell<Compiler>,
     instances: RefCell<Vec<InstanceHandle>>,
+    signal_handler: RefCell<Option<Box<SignalHandler<'static>>>>,
 }
 
 impl Store {
@@ -574,6 +577,7 @@ impl Store {
                 engine: engine.clone(),
                 compiler: RefCell::new(compiler),
                 instances: RefCell::new(Vec::new()),
+                signal_handler: RefCell::new(None),
             }),
         }
     }
@@ -625,6 +629,16 @@ impl Store {
         Rc::downgrade(&self.inner)
     }
 
+    pub(crate) fn signal_handler(&self) -> std::cell::Ref<'_, Option<Box<SignalHandler<'static>>>> {
+        self.inner.signal_handler.borrow()
+    }
+
+    pub(crate) fn signal_handler_mut(
+        &self,
+    ) -> std::cell::RefMut<'_, Option<Box<SignalHandler<'static>>>> {
+        self.inner.signal_handler.borrow_mut()
+    }
+
     /// Returns whether the stores `a` and `b` refer to the same underlying
     /// `Store`.
     ///

crates/api/src/unix.rs

@@ -9,10 +9,10 @@
 //! throughout the `wasmtime` crate with extra functionality that's only
 //! available on Unix.
 
-use crate::Instance;
+use crate::Store;
 
-/// Extensions for the [`Instance`] type only available on Unix.
-pub trait InstanceExt {
+/// Extensions for the [`Store`] type only available on Unix.
+pub trait StoreExt {
     // TODO: needs more docs?
     /// The signal handler must be
     /// [async-signal-safe](http://man7.org/linux/man-pages/man7/signal-safety.7.html).
@@ -21,11 +21,11 @@ pub trait InstanceExt {
         H: 'static + Fn(libc::c_int, *const libc::siginfo_t, *const libc::c_void) -> bool;
 }
 
-impl InstanceExt for Instance {
+impl StoreExt for Store {
     unsafe fn set_signal_handler<H>(&self, handler: H)
     where
         H: 'static + Fn(libc::c_int, *const libc::siginfo_t, *const libc::c_void) -> bool,
     {
-        self.handle.set_signal_handler(handler);
+        *self.signal_handler_mut() = Some(Box::new(handler));
     }
 }

crates/api/src/windows.rs

@@ -9,10 +9,10 @@
 //! throughout the `wasmtime` crate with extra functionality that's only
 //! available on Windows.
 
-use crate::Instance;
+use crate::Store;
 
-/// Extensions for the [`Instance`] type only available on Windows.
-pub trait InstanceExt {
+/// Extensions for the [`Store`] type only available on Windows.
+pub trait StoreExt {
     /// Configures a custom signal handler to execute.
     ///
     /// TODO: needs more documentation.
@@ -21,11 +21,11 @@ pub trait InstanceExt {
         H: 'static + Fn(winapi::um::winnt::PEXCEPTION_POINTERS) -> bool;
 }
 
-impl InstanceExt for Instance {
+impl StoreExt for Store {
     unsafe fn set_signal_handler<H>(&self, handler: H)
     where
         H: 'static + Fn(winapi::um::winnt::PEXCEPTION_POINTERS) -> bool,
     {
-        self.handle.set_signal_handler(handler);
+        *self.signal_handler_mut() = Some(Box::new(handler));
     }
 }