Repository: bytecodealliance/wasmtime
Pull request #1577 (2020-04-22 16:20:07)

Don't try to handle non-wasmtime segfaults
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.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

- [ ] This has been discussed in issue #..., or if not, please tell us why
  here.
- [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.
- [ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the [code of
conduct](https://github.com/bytecodealliance/wasmtime/blob/master/CODE_OF_CONDUCT.md).
-->


alexcrichton: cc @lostman, I wanted to ping you here about per-instance custom signal handlers (added in https://github.com/bytecodealliance/wasmtime/pull/620). There's a bit of a refactoring here to avoid some unsafe `transmute` calls, but it's not required as part of this PR itself. As a question, though, I was wondering if you could elaborate a bit on y'all's usage of signal handlers? Do you catch signals originating in wasm code itself? Or only in your host code? Additionally, does a per-`Store` signal handler work for you as opposed to a per-instance signal handler?
github-actions[bot]: #### Subscribe to Label Action

cc @peterhuene

<details>
This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

* peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

[Learn more.](https://github.com/bytecodealliance/subscribe-to-label-action)
</details>
lostman: @alexcrichton we are using the custom signal handler for memory access tracking using sigsegv+mprotect.

Setting the signal handler per-store will also work but at first glance seems less clean.

Each handler has an instance specific behavior. So we could set the handler once and add some sort of registry to map segfaults to instances.

Or we could set the handler before running each instance, as I see you've done in the `tests/all/custom_signal_handler.hs`. Given that there might be many instances created using a single store, is that the right way to do it though?

eust-dfinity: @alexcrichton There are some other things I'd like to add, which might give us headache:

1. This commit affects not just SIGSEGV/SIGBUS but other signals as well. Notably division by zero in host code will now trigger panic instead of producing a Trap (maybe this is fully intended - I'm just pointing it out in case it's not). My feeling is that life would be simpler to end users if they got consistent behavior, that is always Trap on attempt to div by zero no matter where the trap originated. In our case, we can probably catch it in our custom signal handler and make the whole system respond properly though...

2. My quick tests indicate, that after these changes signals raised manually get completely ignored (this concerns SIGSEGV, SIGILL, SIGBUS, SIGFPE), e.g. executing libc::raise(libc::SIGFPE) in a host call has no effect.

3. We are actually using raise(SIGILL) to terminate wasmtime's execution (similar as the killswitch in lucet). Is there any other way in wasmtime to terminate? (the newly added interrupt functionality won't work, because we want to terminate on the spot, with guarantee that no other instruction will get executed)

alexcrichton: @lostman thanks for taking a look! One thing I think that's a bit surprising about the instance behavior is that you're not actually guaranteed to run the per-instance signal handler if a signal happens while that instance's code is running. The way it's all set up today only if you *enter* with a particular instance do you get the signal handler registered, but with how wasm modules are linked you're not guaranteed that a signal handler runs. That was one simplification I was hoping to apply here where a per-store signal handler is actually what the implementation is providing because it's the only granularity at which we can guarantee it's run.

I think it's probably best from an API-point-of-view that if your usage of wasmtime can guarantee that within a particular scope there's only one instance involved then it'd be encoded into your signal handler. (largely because it's not something we can guarantee in wasmtime itself)

@eust-dfinity oh so recovering from signals in host code is something we never intended to do. It's not memory safe to arbitrarily longjmp out of the middle of a Rust function, so this is a case where wasmtime isn't quite memory safe today and we're trying to fix it. In general if you want to map things like divide-by-zero in the host to a trap in wasm it's something that will need to explicitly be done rather than relying on the signal handler.

For termination, you're asking how to terminate host code? Note that if longjmp out of your host code is safe for your application you should be able to register your own signal handler to jump out. Wasmtime should defer to it once it realizes that the fault didn't happen in wasm. Otherwise I would recommend using `Result`-propagation in Rust code to "quickly" return back to wasm, although it doesn't have the "zero extra instructions executed" property.
eust-dfinity: Thanks for the explanation! Makes sense.

Putting our problems aside, I still don't understand why libc::raise(SIGSEGV) gets ignored. I tried looking at the execution and it seems to be going through wasmtime's signal handler through "not handled" path, while true segfault or division by zero seem to be not triggering the handler at all.
alexcrichton: Ah yeah so division by zero doesn't raise a signal in Rust because it's defined across all platforms to panic, (otherwise I think it's UB). For `libc::raise(libc::SIGSEGV)` I think I see what's happening. When wasmtime decides to ignore a signal it [returns from the signal handler](https://github.com/bytecodealliance/wasmtime/blob/9d13871bd4d09a2e883ee4122bdd7099a1e7e2d3/crates/runtime/src/traphandlers.rs#L128-L136) thinking it will reexecute the faulting instruction, but in this case there's no faulting instruction so presumably it ends up normally returning from the `raise` syscall. 

I'm not actually sure that there's a way to handle *some* segfaults but still abort with `libc::raise(libc::SIGSEGV)`. I do think though that you can use other non-`SIGSEGV` signals to abort the process.
eust-dfinity: Right, it all makes sense now. Thanks again for the explanation!

Patch:
diff --git a/cranelift/frontend/src/frontend.rs b/cranelift/frontend/src/frontend.rs
index df87010ee..bfc0fd232 100644
--- a/cranelift/frontend/src/frontend.rs
+++ b/cranelift/frontend/src/frontend.rs
@@ -259,7 +259,7 @@ impl<'a> FunctionBuilder<'a> {
         self.handle_ssa_side_effects(side_effects);
     }
 
-    /// Effectively calls seal_block on all blocks in the function.
+    /// Effectively calls seal_block on all unsealed blocks in the function.
     ///
     /// It's more efficient to seal `Block`s as soon as possible, during
     /// translation, but for frontends where this is impractical to do, this
diff --git a/cranelift/frontend/src/ssa.rs b/cranelift/frontend/src/ssa.rs
index cfb85a028..4eef23429 100644
--- a/cranelift/frontend/src/ssa.rs
+++ b/cranelift/frontend/src/ssa.rs
@@ -372,7 +372,7 @@ impl SSABuilder {
         mem::replace(&mut self.side_effects, SideEffects::new())
     }
 
-    /// Completes the global value numbering for all `Block`s in `func`.
+    /// Completes the global value numbering for all unsealed `Block`s in `func`.
     ///
     /// It's more efficient to seal `Block`s as soon as possible, during
     /// translation, but for frontends where this is impractical to do, this
@@ -383,7 +383,9 @@ impl SSABuilder {
         // and creation of new blocks, however such new blocks are sealed on
         // the fly, so we don't need to account for them here.
         for block in self.ssa_blocks.keys() {
-            self.seal_one_block(block, func);
+            if !self.is_sealed(block) {
+                self.seal_one_block(block, func);
+            }
         }
         mem::replace(&mut self.side_effects, SideEffects::new())
     }
diff --git a/cranelift/frontend/src/switch.rs b/cranelift/frontend/src/switch.rs
index f444d9aac..867b4499d 100644
--- a/cranelift/frontend/src/switch.rs
+++ b/cranelift/frontend/src/switch.rs
@@ -160,6 +160,9 @@ impl Switch {
                 bx.ins().brnz(should_take_right_side, right_block, &[]);
                 bx.ins().jump(left_block, &[]);
 
+                bx.seal_block(left_block);
+                bx.seal_block(right_block);
+
                 stack.push((Some(left_block), left));
                 stack.push((Some(right_block), right));
             }
@@ -181,6 +184,7 @@ impl Switch {
             if was_branch {
                 let block = bx.create_block();
                 bx.ins().jump(block, &[]);
+                bx.seal_block(block);
                 bx.switch_to_block(block);
             }
         };
@@ -203,6 +207,7 @@ impl Switch {
                     // if `first_index` is 0, then `icmp_imm uge val, first_index` is trivially true
                     let jt_block = bx.create_block();
                     bx.ins().jump(jt_block, &[]);
+                    bx.seal_block(jt_block);
                     cases_and_jt_blocks.push((first_index, jt_block, blocks));
                     // `jump otherwise` below must not be hit, because the current block has been
                     // filled above. This is the last iteration anyway, as 0 is the smallest
@@ -218,6 +223,7 @@ impl Switch {
                         first_index as i64,
                     );
                     bx.ins().brnz(is_good_val, jt_block, &[]);
+                    bx.seal_block(jt_block);
                     cases_and_jt_blocks.push((first_index, jt_block, blocks));
                 }
             }
@@ -488,4 +494,40 @@ block4:
     br_table.i32 v1, block0, jt0"
         );
     }
+
+    #[test]
+    fn switch_seal_generated_blocks() {
+        let keys = [0, 1, 2, 10, 11, 12, 20, 30, 40, 50];
+
+        let mut func = Function::new();
+        let mut builder_ctx = FunctionBuilderContext::new();
+        let mut builder = FunctionBuilder::new(&mut func, &mut builder_ctx);
+
+        let root_block = builder.create_block();
+        let default_block = builder.create_block();
+        let mut switch = Switch::new();
+
+        let case_blocks = keys
+            .iter()
+            .map(|key| {
+                let block = builder.create_block();
+                switch.set_entry(*key, block);
+                block
+            })
+            .collect::<Vec<_>>();
+
+        builder.seal_block(root_block);
+        builder.switch_to_block(root_block);
+
+        let val = builder.ins().iconst(types::I32, 1);
+        switch.emit(&mut builder, val, default_block);
+
+        for &block in case_blocks.iter().chain(std::iter::once(&default_block)) {
+            builder.seal_block(block);
+            builder.switch_to_block(block);
+            builder.ins().return_(&[]);
+        }
+
+        builder.finalize(); // Will panic if some blocks are not sealed
+    }
 }
diff --git a/crates/api/src/func.rs b/crates/api/src/func.rs
index ac1838a5e..b1b18be7c 100644
--- a/crates/api/src/func.rs
+++ b/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.
 ///
diff --git a/crates/api/src/instance.rs b/crates/api/src/instance.rs
index 02c535e77..808f5d397 100644
--- a/crates/api/src/instance.rs
+++ b/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)
     }
 }
diff --git a/crates/api/src/runtime.rs b/crates/api/src/runtime.rs
index 6592cc93d..d81b11a06 100644
--- a/crates/api/src/runtime.rs
+++ b/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`.
     ///
diff --git a/crates/api/src/unix.rs b/crates/api/src/unix.rs
index 22fa39259..4e2f8c61c 100644
--- a/crates/api/src/unix.rs
+++ b/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));
     }
 }
diff --git a/crates/api/src/windows.rs b/crates/api/src/windows.rs
index 5725007d5..f691e1aa3 100644
--- a/crates/api/src/windows.rs
+++ b/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));
     }
 }
diff --git a/crates/jit/src/code_memory.rs b/crates/jit/src/code_memory.rs
index 6d90c8571..e0b7315c4 100644
--- a/crates/jit/src/code_memory.rs
+++ b/crates/jit/src/code_memory.rs
@@ -21,6 +21,12 @@ impl CodeMemoryEntry {
         let registry = ManuallyDrop::new(UnwindRegistry::new(mmap.as_ptr() as usize));
         Ok(Self { mmap, registry })
     }
+
+    fn contains(&self, addr: usize) -> bool {
+        let start = self.mmap.as_ptr() as usize;
+        let end = start + self.mmap.len();
+        start <= addr && addr < end
+    }
 }
 
 impl Drop for CodeMemoryEntry {
@@ -236,4 +242,12 @@ impl CodeMemory {
 
         Ok(())
     }
+
+    /// Returns whether any published segment of this code memory contains
+    /// `addr`.
+    pub fn published_contains(&self, addr: usize) -> bool {
+        self.entries[..self.published]
+            .iter()
+            .any(|entry| entry.contains(addr))
+    }
 }
diff --git a/crates/jit/src/compiler.rs b/crates/jit/src/compiler.rs
index d0307fc32..0bcef5f0f 100644
--- a/crates/jit/src/compiler.rs
+++ b/crates/jit/src/compiler.rs
@@ -235,6 +235,12 @@ impl Compiler {
     pub fn signatures(&self) -> &SignatureRegistry {
         &self.signatures
     }
+
+    /// Returns whether or not the given address falls within the JIT code
+    /// managed by the compiler
+    pub fn is_in_jit_code(&self, addr: usize) -> bool {
+        self.code_memory.published_contains(addr)
+    }
 }
 
 /// Create a trampoline for invoking a function.
diff --git a/crates/runtime/src/instance.rs b/crates/runtime/src/instance.rs
index bbf4ebfab..1d5b17f95 100644
--- a/crates/runtime/src/instance.rs
+++ b/crates/runtime/src/instance.rs
@@ -8,7 +8,7 @@ use crate::jit_int::GdbJitImageRegistration;
 use crate::memory::{DefaultMemoryCreator, RuntimeLinearMemory, RuntimeMemoryCreator};
 use crate::table::Table;
 use crate::traphandlers;
-use crate::traphandlers::{catch_traps, Trap};
+use crate::traphandlers::Trap;
 use crate::vmcontext::{
     VMBuiltinFunctionsArray, VMCallerCheckedAnyfunc, VMContext, VMFunctionBody, VMFunctionImport,
     VMGlobalDefinition, VMGlobalImport, VMInterrupts, VMMemoryDefinition, VMMemoryImport,
@@ -19,7 +19,7 @@ use memoffset::offset_of;
 use more_asserts::assert_lt;
 use std::alloc::{self, Layout};
 use std::any::Any;
-use std::cell::{Cell, RefCell};
+use std::cell::RefCell;
 use std::collections::HashMap;
 use std::convert::TryFrom;
 use std::rc::Rc;
@@ -33,34 +33,6 @@ use wasmtime_environ::wasm::{
 };
 use wasmtime_environ::{ir, DataInitializer, EntityIndex, Module, TableElements, VMOffsets};
 
-cfg_if::cfg_if! {
-    if #[cfg(unix)] {
-        pub type SignalHandler = dyn Fn(libc::c_int, *const libc::siginfo_t, *const libc::c_void) -> bool;
-
-        impl InstanceHandle {
-            /// Set a custom signal handler
-            pub 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.instance().signal_handler.set(Some(Box::new(handler)));
-            }
-        }
-    } else if #[cfg(target_os = "windows")] {
-        pub type SignalHandler = dyn Fn(winapi::um::winnt::PEXCEPTION_POINTERS) -> bool;
-
-        impl InstanceHandle {
-            /// Set a custom signal handler
-            pub fn set_signal_handler<H>(&self, handler: H)
-            where
-                H: 'static + Fn(winapi::um::winnt::PEXCEPTION_POINTERS) -> bool,
-            {
-                self.instance().signal_handler.set(Some(Box::new(handler)));
-            }
-        }
-    }
-}
-
 /// A WebAssembly instance.
 ///
 /// This is repr(C) to ensure that the vmctx field is last.
@@ -99,9 +71,6 @@ pub(crate) struct Instance {
     /// Optional image of JIT'ed code for debugger registration.
     dbg_jit_registration: Option<Rc<GdbJitImageRegistration>>,
 
-    /// Handler run when `SIGBUS`, `SIGFPE`, `SIGILL`, or `SIGSEGV` are caught by the instance thread.
-    pub(crate) signal_handler: Cell<Option<Box<SignalHandler>>>,
-
     /// Externally allocated data indicating how this instance will be
     /// interrupted.
     pub(crate) interrupts: Arc<VMInterrupts>,
@@ -377,58 +346,6 @@ impl Instance {
         &*self.host_state
     }
 
-    /// Invoke the WebAssembly start function of the instance, if one is present.
-    fn invoke_start_function(&self, max_wasm_stack: usize) -> Result<(), InstantiationError> {
-        let start_index = match self.module.start_func {
-            Some(idx) => idx,
-            None => return Ok(()),
-        };
-
-        self.invoke_function_index(start_index, max_wasm_stack)
-            .map_err(InstantiationError::StartTrap)
-    }
-
-    fn invoke_function_index(
-        &self,
-        callee_index: FuncIndex,
-        max_wasm_stack: usize,
-    ) -> Result<(), Trap> {
-        let (callee_address, callee_vmctx) =
-            match self.module.local.defined_func_index(callee_index) {
-                Some(defined_index) => {
-                    let body = *self
-                        .finished_functions
-                        .get(defined_index)
-                        .expect("function index is out of bounds");
-                    (body as *const _, self.vmctx_ptr())
-                }
-                None => {
-                    assert_lt!(callee_index.index(), self.module.local.num_imported_funcs);
-                    let import = self.imported_function(callee_index);
-                    (import.body, import.vmctx)
-                }
-            };
-
-        self.invoke_function(callee_vmctx, callee_address, max_wasm_stack)
-    }
-
-    fn invoke_function(
-        &self,
-        callee_vmctx: *mut VMContext,
-        callee_address: *const VMFunctionBody,
-        max_wasm_stack: usize,
-    ) -> Result<(), Trap> {
-        // Make the call.
-        unsafe {
-            catch_traps(callee_vmctx, max_wasm_stack, || {
-                mem::transmute::<
-                    *const VMFunctionBody,
-                    unsafe extern "C" fn(*mut VMContext, *mut VMContext),
-                >(callee_address)(callee_vmctx, self.vmctx_ptr())
-            })
-        }
-    }
-
     /// Return the offset from the vmctx pointer to its containing Instance.
     #[inline]
     pub(crate) fn vmctx_offset() -> isize {
@@ -908,7 +825,6 @@ impl InstanceHandle {
                 trampolines,
                 dbg_jit_registration,
                 host_state,
-                signal_handler: Cell::new(None),
                 interrupts,
                 vmctx: VMContext {},
             };
@@ -988,7 +904,6 @@ impl InstanceHandle {
     pub unsafe fn initialize(
         &self,
         is_bulk_memory: bool,
-        max_wasm_stack: usize,
         data_initializers: &[DataInitializer<'_>],
     ) -> Result<(), InstantiationError> {
         // Check initializer bounds before initializing anything. Only do this
@@ -1005,8 +920,6 @@ impl InstanceHandle {
         initialize_tables(self.instance())?;
         initialize_memories(self.instance(), data_initializers)?;
 
-        // And finally, invoke the start function.
-        self.instance().invoke_start_function(max_wasm_stack)?;
         Ok(())
     }
 
diff --git a/crates/runtime/src/lib.rs b/crates/runtime/src/lib.rs
index 94816f6f1..395135596 100644
--- a/crates/runtime/src/lib.rs
+++ b/crates/runtime/src/lib.rs
@@ -43,8 +43,9 @@ pub use crate::memory::{RuntimeLinearMemory, RuntimeMemoryCreator};
 pub use crate::mmap::Mmap;
 pub use crate::sig_registry::SignatureRegistry;
 pub use crate::table::Table;
-pub use crate::traphandlers::resume_panic;
-pub use crate::traphandlers::{catch_traps, raise_lib_trap, raise_user_trap, Trap};
+pub use crate::traphandlers::{
+    catch_traps, raise_lib_trap, raise_user_trap, resume_panic, SignalHandler, Trap,
+};
 pub use crate::vmcontext::{
     VMCallerCheckedAnyfunc, VMContext, VMFunctionBody, VMFunctionImport, VMGlobalDefinition,
     VMGlobalImport, VMInterrupts, VMInvokeArgument, VMMemoryDefinition, VMMemoryImport,
diff --git a/crates/runtime/src/traphandlers.rs b/crates/runtime/src/traphandlers.rs
index 488dc3a05..d21568b90 100644
--- a/crates/runtime/src/traphandlers.rs
+++ b/crates/runtime/src/traphandlers.rs
@@ -1,7 +1,6 @@
 //! WebAssembly trap handling, which is built on top of the lower-level
 //! signalhandling mechanisms.
 
-use crate::instance::{InstanceHandle, SignalHandler};
 use crate::VMContext;
 use backtrace::Backtrace;
 use std::any::Any;
@@ -26,6 +25,9 @@ cfg_if::cfg_if! {
     if #[cfg(unix)] {
         use std::mem::{self, MaybeUninit};
 
+        /// Function which may handle custom signals while processing traps.
+        pub type SignalHandler<'a> = dyn Fn(libc::c_int, *const libc::siginfo_t, *const libc::c_void) -> bool + 'a;
+
         static mut PREV_SIGSEGV: MaybeUninit<libc::sigaction> = MaybeUninit::uninit();
         static mut PREV_SIGBUS: MaybeUninit<libc::sigaction> = MaybeUninit::uninit();
         static mut PREV_SIGILL: MaybeUninit<libc::sigaction> = MaybeUninit::uninit();
@@ -180,6 +182,9 @@ cfg_if::cfg_if! {
         use winapi::um::minwinbase::*;
         use winapi::vc::excpt::*;
 
+        /// Function which may handle custom signals while processing traps.
+        pub type SignalHandler<'a> = dyn Fn(winapi::um::winnt::PEXCEPTION_POINTERS) -> bool + 'a;
+
         unsafe fn platform_init() {
             // our trap handler needs to go first, so that we can recover from
             // wasm faults and continue execution, so pass `1` as a true value
@@ -361,6 +366,8 @@ impl Trap {
 pub unsafe fn catch_traps<F>(
     vmctx: *mut VMContext,
     max_wasm_stack: usize,
+    is_wasm_code: impl Fn(usize) -> bool,
+    signal_handler: Option<&SignalHandler>,
     mut closure: F,
 ) -> Result<(), Trap>
 where
@@ -370,7 +377,7 @@ where
     #[cfg(unix)]
     setup_unix_sigaltstack()?;
 
-    return CallThreadState::new(vmctx).with(max_wasm_stack, |cx| {
+    return CallThreadState::new(vmctx, &is_wasm_code, signal_handler).with(max_wasm_stack, |cx| {
         RegisterSetjmp(
             cx.jmp_buf.as_ptr(),
             call_closure::<F>,
@@ -388,12 +395,13 @@ where
 
 /// Temporary state stored on the stack which is registered in the `tls` module
 /// below for calls into wasm.
-pub struct CallThreadState {
+pub struct CallThreadState<'a> {
     unwind: Cell<UnwindReason>,
     jmp_buf: Cell<*const u8>,
-    prev: Option<*const CallThreadState>,
     vmctx: *mut VMContext,
     handling_trap: Cell<bool>,
+    is_wasm_code: &'a (dyn Fn(usize) -> bool + 'a),
+    signal_handler: Option<&'a SignalHandler<'a>>,
 }
 
 enum UnwindReason {
@@ -404,54 +412,56 @@ enum UnwindReason {
     JitTrap { backtrace: Backtrace, pc: usize },
 }
 
-impl CallThreadState {
-    fn new(vmctx: *mut VMContext) -> CallThreadState {
+impl<'a> CallThreadState<'a> {
+    fn new(
+        vmctx: *mut VMContext,
+        is_wasm_code: &'a (dyn Fn(usize) -> bool + 'a),
+        signal_handler: Option<&'a SignalHandler<'a>>,
+    ) -> CallThreadState<'a> {
         CallThreadState {
             unwind: Cell::new(UnwindReason::None),
             vmctx,
             jmp_buf: Cell::new(ptr::null()),
-            prev: None,
             handling_trap: Cell::new(false),
+            is_wasm_code,
+            signal_handler,
         }
     }
 
     fn with(
-        mut self,
+        self,
         max_wasm_stack: usize,
         closure: impl FnOnce(&CallThreadState) -> i32,
     ) -> Result<(), Trap> {
-        tls::with(|prev| {
-            self.prev = prev.map(|p| p as *const _);
-            let _reset = self.update_stack_limit(max_wasm_stack)?;
-            let ret = tls::set(&self, || closure(&self));
-            match self.unwind.replace(UnwindReason::None) {
-                UnwindReason::None => {
-                    debug_assert_eq!(ret, 1);
-                    Ok(())
-                }
-                UnwindReason::UserTrap(data) => {
-                    debug_assert_eq!(ret, 0);
-                    Err(Trap::User(data))
-                }
-                UnwindReason::LibTrap(trap) => Err(trap),
-                UnwindReason::JitTrap { backtrace, pc } => {
-                    debug_assert_eq!(ret, 0);
-                    let maybe_interrupted = unsafe {
-                        (*self.vmctx).instance().interrupts.stack_limit.load(SeqCst)
-                            == wasmtime_environ::INTERRUPTED
-                    };
-                    Err(Trap::Jit {
-                        pc,
-                        backtrace,
-                        maybe_interrupted,
-                    })
-                }
-                UnwindReason::Panic(panic) => {
-                    debug_assert_eq!(ret, 0);
-                    std::panic::resume_unwind(panic)
-                }
+        let _reset = self.update_stack_limit(max_wasm_stack)?;
+        let ret = tls::set(&self, || closure(&self));
+        match self.unwind.replace(UnwindReason::None) {
+            UnwindReason::None => {
+                debug_assert_eq!(ret, 1);
+                Ok(())
             }
-        })
+            UnwindReason::UserTrap(data) => {
+                debug_assert_eq!(ret, 0);
+                Err(Trap::User(data))
+            }
+            UnwindReason::LibTrap(trap) => Err(trap),
+            UnwindReason::JitTrap { backtrace, pc } => {
+                debug_assert_eq!(ret, 0);
+                let maybe_interrupted = unsafe {
+                    (*self.vmctx).instance().interrupts.stack_limit.load(SeqCst)
+                        == wasmtime_environ::INTERRUPTED
+                };
+                Err(Trap::Jit {
+                    pc,
+                    backtrace,
+                    maybe_interrupted,
+                })
+            }
+            UnwindReason::Panic(panic) => {
+                debug_assert_eq!(ret, 0);
+                std::panic::resume_unwind(panic)
+            }
+        }
     }
 
     /// Checks and/or initializes the wasm native call stack limit.
@@ -535,18 +545,6 @@ impl CallThreadState {
         Ok(Reset(reset_stack_limit, &interrupts.stack_limit))
     }
 
-    fn any_instance(&self, func: impl Fn(&InstanceHandle) -> bool) -> bool {
-        unsafe {
-            if func(&InstanceHandle::from_vmctx(self.vmctx)) {
-                return true;
-            }
-            match self.prev {
-                Some(prev) => (*prev).any_instance(func),
-                None => false,
-            }
-        }
-    }
-
     fn unwind_with(&self, reason: UnwindReason) -> ! {
         self.unwind.replace(reason);
         unsafe {
@@ -582,21 +580,25 @@ impl CallThreadState {
         if self.handling_trap.replace(true) {
             return ptr::null();
         }
+        let _reset = ResetCell(&self.handling_trap, false);
+
+        // If we haven't even started to handle traps yet, bail out.
+        if self.jmp_buf.get().is_null() {
+            return ptr::null();
+        }
+
+        // If this fault wasn't in wasm code, then it's not our problem
+        if !(self.is_wasm_code)(pc as usize) {
+            return ptr::null();
+        }
 
         // First up see if any instance registered has a custom trap handler,
         // in which case run them all. If anything handles the trap then we
         // return that the trap was handled.
-        if self.any_instance(|i| {
-            let handler = match i.instance().signal_handler.replace(None) {
-                Some(handler) => handler,
-                None => return false,
-            };
-            let result = call_handler(&handler);
-            i.instance().signal_handler.set(Some(handler));
-            return result;
-        }) {
-            self.handling_trap.set(false);
-            return 1 as *const _;
+        if let Some(handler) = self.signal_handler {
+            if call_handler(handler) {
+                return 1 as *const _;
+            }
         }
 
         // TODO: stack overflow can happen at any random time (i.e. in malloc()
@@ -607,7 +609,6 @@ impl CallThreadState {
         // doesn't trap. Then, if we have called some WebAssembly code, it
         // means the trap is stack overflow.
         if self.jmp_buf.get().is_null() {
-            self.handling_trap.set(false);
             return ptr::null();
         }
         let backtrace = Backtrace::new_unresolved();
@@ -615,11 +616,18 @@ impl CallThreadState {
             backtrace,
             pc: pc as usize,
         });
-        self.handling_trap.set(false);
         self.jmp_buf.get()
     }
 }
 
+struct ResetCell<'a, T: Copy>(&'a Cell<T>, T);
+
+impl<T: Copy> Drop for ResetCell<'_, T> {
+    fn drop(&mut self) {
+        self.0.set(self.1);
+    }
+}
+
 // A private inner module for managing the TLS state that we require across
 // calls in wasm. The WebAssembly code is called from C++ and then a trap may
 // happen which requires us to read some contextual state to figure out what to
@@ -628,14 +636,15 @@ impl CallThreadState {
 mod tls {
     use super::CallThreadState;
     use std::cell::Cell;
+    use std::mem;
     use std::ptr;
 
-    thread_local!(static PTR: Cell<*const CallThreadState> = Cell::new(ptr::null()));
+    thread_local!(static PTR: Cell<*const CallThreadState<'static>> = Cell::new(ptr::null()));
 
     /// Configures thread local state such that for the duration of the
     /// execution of `closure` any call to `with` will yield `ptr`, unless this
     /// is recursively called again.
-    pub fn set<R>(ptr: &CallThreadState, closure: impl FnOnce() -> R) -> R {
+    pub fn set<R>(ptr: &CallThreadState<'_>, closure: impl FnOnce() -> R) -> R {
         struct Reset<'a, T: Copy>(&'a Cell<T>, T);
 
         impl<T: Copy> Drop for Reset<'_, T> {
@@ -645,6 +654,12 @@ mod tls {
         }
 
         PTR.with(|p| {
+            // Note that this extension of the lifetime to `'static` should be
+            // safe because we only ever access it below with an anonymous
+            // lifetime, meaning `'static` never leaks out of this module.
+            let ptr = unsafe {
+                mem::transmute::<*const CallThreadState<'_>, *const CallThreadState<'static>>(ptr)
+            };
             let _r = Reset(p, p.replace(ptr));
             closure()
         })
@@ -652,7 +667,7 @@ mod tls {
 
     /// Returns the last pointer configured with `set` above. Panics if `set`
     /// has not been previously called.
-    pub fn with<R>(closure: impl FnOnce(Option<&CallThreadState>) -> R) -> R {
+    pub fn with<R>(closure: impl FnOnce(Option<&CallThreadState<'_>>) -> R) -> R {
         PTR.with(|ptr| {
             let p = ptr.get();
             unsafe { closure(if p.is_null() { None } else { Some(&*p) }) }


Test patch:
diff --git a/tests/all/custom_signal_handler.rs b/tests/all/custom_signal_handler.rs
index c75821ef1..ba5a96b16 100644
--- a/tests/all/custom_signal_handler.rs
+++ b/tests/all/custom_signal_handler.rs
@@ -3,7 +3,7 @@ mod tests {
     use anyhow::Result;
     use std::rc::Rc;
     use std::sync::atomic::{AtomicBool, Ordering};
-    use wasmtime::unix::InstanceExt;
+    use wasmtime::unix::StoreExt;
     use wasmtime::*;
 
     const WAT1: &str = r#"
@@ -96,7 +96,7 @@ mod tests {
 
         let (base, length) = set_up_memory(&instance);
         unsafe {
-            instance.set_signal_handler(move |signum, siginfo, _| {
+            store.set_signal_handler(move |signum, siginfo, _| {
                 handle_sigsegv(base, length, signum, siginfo)
             });
         }
@@ -161,7 +161,7 @@ mod tests {
         unsafe {
             let (base1, length1) = set_up_memory(&instance1);
 
-            instance1.set_signal_handler({
+            store.set_signal_handler({
                 let instance1_handler_triggered = instance1_handler_triggered.clone();
                 move |_signum, _siginfo, _context| {
                     // Remove protections so the execution may resume
@@ -180,13 +180,30 @@ mod tests {
             });
         }
 
+        // Invoke both instances and trigger both signal handlers
+
+        // First instance1
+        {
+            let mut exports1 = instance1.exports();
+            assert!(exports1.next().is_some());
+
+            println!("calling instance1.read...");
+            let result = invoke_export(&instance1, "read").expect("read succeeded");
+            assert_eq!(123, result[0].unwrap_i32());
+            assert_eq!(
+                instance1_handler_triggered.load(Ordering::SeqCst),
+                true,
+                "instance1 signal handler has been triggered"
+            );
+        }
+
         let instance2 = Instance::new(&module, &[]).expect("failed to instantiate module");
         let instance2_handler_triggered = Rc::new(AtomicBool::new(false));
 
         unsafe {
             let (base2, length2) = set_up_memory(&instance2);
 
-            instance2.set_signal_handler({
+            store.set_signal_handler({
                 let instance2_handler_triggered = instance2_handler_triggered.clone();
                 move |_signum, _siginfo, _context| {
                     // Remove protections so the execution may resume
@@ -205,23 +222,6 @@ mod tests {
             });
         }
 
-        // Invoke both instances and trigger both signal handlers
-
-        // First instance1
-        {
-            let mut exports1 = instance1.exports();
-            assert!(exports1.next().is_some());
-
-            println!("calling instance1.read...");
-            let result = invoke_export(&instance1, "read").expect("read succeeded");
-            assert_eq!(123, result[0].unwrap_i32());
-            assert_eq!(
-                instance1_handler_triggered.load(Ordering::SeqCst),
-                true,
-                "instance1 signal handler has been triggered"
-            );
-        }
-
         // And then instance2
         {
             let mut exports2 = instance2.exports();
@@ -249,7 +249,7 @@ mod tests {
         let instance1 = Instance::new(&module1, &[])?;
         let (base1, length1) = set_up_memory(&instance1);
         unsafe {
-            instance1.set_signal_handler(move |signum, siginfo, _| {
+            store.set_signal_handler(move |signum, siginfo, _| {
                 println!("instance1");
                 handle_sigsegv(base1, length1, signum, siginfo)
             });
@@ -264,7 +264,7 @@ mod tests {
         // since 'instance2.run' calls 'instance1.read' we need to set up the signal handler to handle
         // SIGSEGV originating from within the memory of instance1
         unsafe {
-            instance2.set_signal_handler(move |signum, siginfo, _| {
+            store.set_signal_handler(move |signum, siginfo, _| {
                 handle_sigsegv(base1, length1, signum, siginfo)
             });
         }
diff --git a/tests/host_segfault.rs b/tests/host_segfault.rs
index cc29fe87d..188496728 100644
--- a/tests/host_segfault.rs
+++ b/tests/host_segfault.rs
@@ -59,6 +59,12 @@ fn main() {
             let _instance = Instance::new(&module, &[]).unwrap();
             println!("stack overrun: {}", overrun_the_stack());
         }),
+        ("segfault in a host function", || {
+            let store = Store::default();
+            let module = Module::new(&store, r#"(import "" "" (func)) (start 0)"#).unwrap();
+            let segfault = Func::wrap(&store, || segfault());
+            Instance::new(&module, &[segfault.into()]).unwrap();
+        }),
     ];
     match env::var(VAR_NAME) {
         Ok(s) => {