From f12ef84cdc88aa6be44117e4dc26886dccc8b91f Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 26 Sep 2022 17:41:47 -0500 Subject: [PATCH] Remove `handling_trap` variable (#4963) This historically was used to guard against recursive faults but later refactorings have made this variable somewhat obsolete. The code that it still protects is not the "meat" of trap handling. Instead the `jmp_buf_if_trap` is changed to be more like "take" so once a "take" succeeds it won't be able to recursively call any more "meat". Overall this shouldn't affect anything, it's just a small internal cleanup. --- crates/runtime/src/traphandlers.rs | 16 ++-------------- crates/runtime/src/traphandlers/unix.rs | 2 +- crates/runtime/src/traphandlers/windows.rs | 2 +- 3 files changed, 4 insertions(+), 16 deletions(-) diff --git a/crates/runtime/src/traphandlers.rs b/crates/runtime/src/traphandlers.rs index a08a0507f4..3a24db44af 100644 --- a/crates/runtime/src/traphandlers.rs +++ b/crates/runtime/src/traphandlers.rs @@ -216,7 +216,6 @@ mod call_thread_state { pub struct CallThreadState { pub(super) unwind: UnsafeCell)>>, pub(super) jmp_buf: Cell<*const u8>, - pub(super) handling_trap: Cell, pub(super) signal_handler: Option<*const SignalHandler<'static>>, pub(super) capture_backtrace: bool, @@ -246,7 +245,6 @@ mod call_thread_state { CallThreadState { unwind: UnsafeCell::new(MaybeUninit::uninit()), jmp_buf: Cell::new(ptr::null()), - handling_trap: Cell::new(false), signal_handler, capture_backtrace, limits, @@ -406,21 +404,11 @@ impl CallThreadState { /// * a different pointer - a jmp_buf buffer to longjmp to, meaning that /// the wasm trap was succesfully handled. #[cfg_attr(target_os = "macos", allow(dead_code))] // macOS is more raw and doesn't use this - fn jmp_buf_if_trap( + fn take_jmp_buf_if_trap( &self, pc: *const u8, call_handler: impl Fn(&SignalHandler) -> bool, ) -> *const u8 { - // If we hit a fault while handling a previous trap, that's quite bad, - // so bail out and let the system handle this recursive segfault. - // - // Otherwise flag ourselves as handling a trap, do the trap handling, - // and reset our trap handling flag. - 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(); @@ -442,7 +430,7 @@ impl CallThreadState { // If all that passed then this is indeed a wasm trap, so return the // `jmp_buf` passed to `wasmtime_longjmp` to resume. - self.jmp_buf.get() + self.jmp_buf.replace(ptr::null()) } fn set_jit_trap(&self, pc: *const u8, fp: usize) { diff --git a/crates/runtime/src/traphandlers/unix.rs b/crates/runtime/src/traphandlers/unix.rs index be3b13d14a..ef40b8c2c3 100644 --- a/crates/runtime/src/traphandlers/unix.rs +++ b/crates/runtime/src/traphandlers/unix.rs @@ -87,7 +87,7 @@ unsafe extern "C" fn trap_handler( // handling, and reset our trap handling flag. Then we figure // out what to do based on the result of the trap handling. let (pc, fp) = get_pc_and_fp(context, signum); - let jmp_buf = info.jmp_buf_if_trap(pc, |handler| handler(signum, siginfo, context)); + let jmp_buf = info.take_jmp_buf_if_trap(pc, |handler| handler(signum, siginfo, context)); // Figure out what to do based on the result of this handling of // the trap. Note that our sentinel value of 1 means that the diff --git a/crates/runtime/src/traphandlers/windows.rs b/crates/runtime/src/traphandlers/windows.rs index b89910ed41..8931503fec 100644 --- a/crates/runtime/src/traphandlers/windows.rs +++ b/crates/runtime/src/traphandlers/windows.rs @@ -62,7 +62,7 @@ unsafe extern "system" fn exception_handler(exception_info: *mut EXCEPTION_POINT compile_error!("unsupported platform"); } } - let jmp_buf = info.jmp_buf_if_trap(ip, |handler| handler(exception_info)); + let jmp_buf = info.take_jmp_buf_if_trap(ip, |handler| handler(exception_info)); if jmp_buf.is_null() { ExceptionContinueSearch } else if jmp_buf as usize == 1 {