Make try_vm_runtime panic hook thread-safe

[eranrund]

Motivation

Make try_vm_runtime safe under concurrent execution. Fixes #3327.

Problem

try_vm_runtime! does a take_hook() / set_hook() / restore dance around its catch_unwind. The panic hook is a process-wide singleton, so running VM operations on multiple threads concurrently races these swaps: a VM halt on one thread can be reported by whatever hook another thread happens to have installed, debug builds leak VM safely halted at ... nondeterministically, and the swaps can interleave such that a host-installed panic hook is silently dropped. Results are unaffected (catch_unwind still catches). See #3327 for details.

Fix

Install one persistent, VM-aware panic hook (guarded by a Once) and gate the "VM safely halted" handling on a thread-local IN_VM_RUNTIME flag instead of mutating the global hook per operation:

• try_vm_runtime sets the flag around its catch_unwind and restores the previous value afterwards, so nested calls work. I don't think we currently worry about nested calls but it's easy to support this way.
• When the flag is set on the panicking thread, the hook prints VM safely halted ..., preserving the existing behavior.
• When the flag is not set - i.e. the panic is unrelated to VM execution - the hook delegates to the previously-installed hook, so unrelated panics keep the default print/backtrace behavior or any previously-installed hook.

Relation to #2927

This is an alternative to the panic-hook portion of #2927, which also installs a single persistent hook but takes the opposite approach to reporting: its hook never prints and never delegates - it captures the panic message and backtrace into a thread-local so the catcher (try_vm_runtime / catch_unwind) can log them. A consequence is that any panic not wrapped by those helpers becomes completely silent, since the previous/default hook is replaced and the stored info is never read.

The position taken here is to keep the two concerns separate: the panic hook is the right place for reporting (it runs exactly once, at the panic site, with the backtrace available), while catch_unwind / JoinHandle wrappers are for control flow (propagation, deciding to shut down). Since resume_unwind does not re-invoke the hook, a propagated panic is reported exactly once and nothing needs to carry the backtrace as data.

This PR does not touch the task-management side of #2927 (the tokio::spawn/spawn_blocking wrappers in utilities/src/task.rs); those are orthogonal and compose with this change.

Test Plan

Unit tests cover success/panic payload preservation, nesting, and a multi-threaded test (run in a child process, since hook and Once state are process-global) asserting that 8 concurrent VM halts each print exactly once while an unrelated panic on the same threads still reaches the host's hook.

Documentation

N/A

Backwards compatibility

This should be fully backwards compatible.

[eranrund]

This test is pretty convoluted, but dealing with global state is usually messy. I'm open for suggestions on other ways to do it, or to remove it entirely if we decide it's a maintenance burden.

[ljedrz]

This is an overall improvement, with one caveat - I think this change could cause halts in the context of rayon's worker threads to not be graceful (i.e. the scary default panic message would be displayed instead), unless the entire closure is wrapped in try_vm_runtime.

Also left one small hardening suggestion.

[eranrund]

This is an overall improvement, with one caveat - I think this change could cause halts in the context of rayon's worker threads to not be graceful (i.e. the scary default panic message would be displayed instead), unless the entire closure is wrapped in try_vm_runtime.

Also left one small hardening suggestion.

Thank you @ljedrz !

You're right about the rayon worker threads. I need to give it some more thought. Off the top of my head I can think of a few possible approaches (this is not fully fleshed out):

1. Surgically wrapping closures in try_vm_runtime at the parallel call sites where we know we're in VM code, like you suggested (re-raising the payload with resume_unwind so propagation to the outer catch is preserved). I think this might still not be optimal: I think that if the rayon worker blocks, it can switch and execute an unrelated task while its thread-local flag is still set, so a panic in that task would be wrongly silenced as a VM halt. Cosmetic-only but still not optimal.

2. A dedicated rayon thread pool for VM executions that uses start_handler to set IN_VM_RUNTIME = true on every worker. Pool routing would take place inside try_vm_runtime (VM_POOL.install(f)). I imagine there are some gotchas, at the very least this will result in twice as many Rayon worker threads...

3. Prioritizing [Bug] Remove clear panic potentials in validator code paths #2941 to systematically get rid of panics. A lot of work, and it can't fully eliminate panics because the std::ops trait signatures (Div, Rem, ...) force Output = T rather than Result - so those impls must halt via panic forever. On the flip side, once [Bug] Remove clear panic potentials in validator code paths #2941 lands, any panic that isn't one of those ops halts would be a genuine bug, and the scary message is arguably the correct output for it. There might be a way to improve on this, see:

4. Tagging halts by panic payload type. We will need to make sure that every halt - including the Div/Rem by-zero cases and such - funnels through the handful of E::halt impls. Changing those impls from panic!("{}", msg) to panic::panic_any(VmHalt(msg)) would let the hook check info.payload().downcast_ref::<VmHalt>() in addition to the thread-local flag. These halt then identifies itself on any thread. The ops-signature problem from (3) doesn't apply here: those impls can't return Result, but they can panic_any a typed payload.

5. Spawning a child process and handing all VM execution to it over some IPC channel. Extra complication and a likely performance penalty - process isolation feels like the tool for crash/soundness isolation, not for getting log messages right.

I'm leaning towards trying to at the very least locate places inside try_vm_runtime that hand off to Rayon like you suggested as the first step. After that I am open to exploring the other directions. Curious to hear your thoughts on this!

[Antonio95]

Cool solution. The logic seems solid, although several of its building blocks are new to me. Regarding rayon not using the graceful handlers, I see several solutions have been listed. Would it be possible to simply wrap some rayon macros (e.g. cfg_iter) in our own which incorporate the handler/IN_VM_RUNTIME management?

Green light from me, I defer to @eranrund, @ljedrz and more knowledgeable pleople on the rayon topic.

[eranrund]

Thank you @Antonio95

Unfortunately I don't think there's a trivial path where wrapping/changing the cfg_iter macro solves this. There are two obstacles. First, the macro only constructs the iterator - the actual work (e.g. the .for_each closure) executes much later, so there's nothing at the macro site to wrap around. Second, and more fundamentally, IN_VM_RUNTIME is a thread-local, and rayon executes the closures on its pool's worker threads, which don't inherit thread-locals from the calling thread - so even wrapping the execution at the call site wouldn't propagate the flag to the threads that actually run the code. It's possible to provide a custom .par_iter()-like method returning a type that implements rayon's iterator traits and intercepts execution deep enough to set up state on each worker, but my intuition is it is non-trivial.

The least intrusive option I can think of is a dedicated rayon thread pool for VM operations: try_vm_runtime would run its closure via ThreadPool::install, so any par_iter reached from VM code runs on that pool, and the pool's start_handler can set IN_VM_RUNTIME on every worker thread at spawn time. I haven't tried it yet, and it has its own complications - at minimum it is hard to reason about the potential effects of having double the amount of worker threads in the process.