Wasmtime C API: Possibility of switching to "abort" panic mode?

[kpreisser]

Hi!

(Please note that I don't know much about Rust, so please correct me if I'm saying anything that doesn't make sense.)

As far as I understand panics in Rust, they indicate that an unrecoverable/serious error has occured, after which the process should be terminated (to not run into undefined behavior).

In bytecodealliance/wasmtime-dotnet#192, we found that on Windows, when a panic occurs in the Wasmtime C API, it will raise an SEH Exception (e.g. with Win32's RaiseException or _CxxThrowException from the MSVC runtime).

However, because .NET appears to use the same mechanism to handle exceptions, such a panic will surface as SEHException in a .NET application on the managed-to-native transition, which can be caught by the user if they have e.g. an catch (Exception) or catch (SEHException) clause. This means that in such a case, the process will not actually terminate, but will can continue to run, which could be problematic because we may now have undefined behavior with possible security implications.

This can even happen in wasmtime-dotnet if user code actually doesn't intend to catch SEHException:
When a .NET exception occurs in a wasm callback, wasmtime-dotnet will catch the exception (by using an catch (Exception ex) clause to catch all .NET exceptions), and transform it into a wasm_trap_t*, which is then returned at the native wasmtime callback. ¹
Now, imagine there is a host-to-wasm and a wasm-to-host transition on the stack, and you call a wasmtime function that panics, resulting in a SEHException on Windows on the managed-to-native transition. Even if the user code on top of the wasm-to-host transition doesn't have a catch (Exception) or catch (SEHException) clause, the SEHException will be caught by Function's callback handler and transformed into a wasm_trap_t*, which is then reported at the managed-to-native transition (like wasmtime_call_func) as wasmtime_error_t*, and that is thrown in .NET code as a WasmtimeException.
So even if you just use catch (WasmtimeException ...) e.g. around Function.Invoke() (as it is expected that such an exception may be thrown here), it can happen that you catch a Rust panic that was actually intended to terminate the process.

On Windows there is an alternative function RaiseFailFastException, which bypasses all exception handlers and ensures the process is terminated (I assume this is also e.g. what .NET internally does in Environment.FailFast()).
From Rust PR rust-lang/rust#32900, I understand that there is another panic mode (abort) which would have a similar effect to calling RaiseFailFastException.

Quoting @peterhuene from bytecodealliance/wasmtime-dotnet#192 (comment) (please see the conversation there for more background on this in wasmtime-dotnet):

That said, I do think changing the panic mode for the C API to abort would make the most sense to address this issue. Would you mind opening an issue in the Wasmtime repo to explore doing so and reference this conversation?

Would it be possible to change the panic mode for the C API to abort, to ensure on Windows the process is terminated when a panic occurs?
(Note that e.g. on Linux, this is what already happens in .NET applications using wasmtime-dotnet, since the CLR cannot catch the panic there.)

Thank you!

Footnotes

1. This ensures we don't let any .NET exception bubble through the native-to-managed transition. If wasmtime-dotnet wouldn't catch an exception thrown in .NET code, on Windows, the .NET CLR would unwind the stack up to the next .NET exception handler (that catches this exception type) even if there are managed-to-native and native-to-managed transitions on the stack, which seems to be incompatible with Wasmtime - see Undefined behavior when callback exception handler throws another exception wasmtime-dotnet#187 for an example. ↩

[bjorn3]

Using panic=abort for the C api is not enough. You also need to catch exceptions thrown from the .NET side as unwinding through extern "C" is UB. In the future unwinding out from an extern "C" rust function will abort independent of the panic mode, but unwinding into rust from a function declared as extern "C" is still UB.

[kpreisser]

Hi @bjorn3, thanks for your reply!

You also need to catch exceptions thrown from the .NET side as unwinding through extern "C" is UB.

If I understand you correctly, that should already be the case in wasmtime-dotnet, as it uses a catch (Exception ex) clause at wasmtime callbacks (e.g. defined with wasmtime_func_new or wasmtime_func_new_unchecked), to ensure no unwinding can happen beyond the native-to-managed transition (see the referenced issue in the footnote for an example where this still happend previously but has been fixed).

Thanks!

[alexcrichton]

I agree that changing to panic=abort is the best solution here. This will get a bit tricky with CI since it will require a lot of new artifacts to be built (can't share panic=abort and panic=unwind), but otherwise should be easy enough to configure at least.

[Muon]

As far as I understand panics in Rust, they indicate that an unrecoverable/serious error has occured, after which the process should be terminated (to not run into undefined behavior).

To my understanding, this is not the case. A panic is a sign that whatever you tried to do was incorrect, and that logical invariants may not be upheld any more. However, catch_unwind() is not unsafe. Therefore, if you panic in a safe function, it's your responsibility to ensure that UB cannot happen if the panic is caught and execution continues.

[rockwotj]

I'm not sure if this is the right issue, but I'm using the C API and would like to handle panics. I'm seeing cases where there are panics if there is no memory left to allocate memory:

Sep 14 14:49:23 rp-node-0-overly-top-warthog rpk[21379]: thread '<unnamed>' panicked at 'unable to make memory executable: failed to make memory executable
Sep 14 14:49:23 rp-node-0-overly-top-warthog rpk[21379]: Caused by:
Sep 14 14:49:23 rp-node-0-overly-top-warthog rpk[21379]:     Cannot allocate memory (os error 12)', crates/jit/src/code_memory.rs:254:18
Sep 14 14:49:23 rp-node-0-overly-top-warthog rpk[21379]: note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Sep 14 14:49:23 rp-node-0-overly-top-warthog rpk[21379]: fatal runtime error: Rust panics must be rethrown
Sep 14 14:49:23 rp-node-0-overly-top-warthog rpk[21379]: Aborting.

I'd like to handle that and not crash the process. Would patches be accepted to handle this specific case in wasmtime_module_new and return an error?

wasmtime/crates/c-api/src/module.rs

Lines 32 to 40 in c4f261a

	pub unsafe extern "C" fn wasm_module_new(
	store: &mut wasm_store_t,
	binary: &wasm_byte_vec_t,
	) -> Option<Box<wasm_module_t>> {
	match Module::from_binary(store.store.context().engine(), binary.as_slice()) {
	Ok(module) => Some(Box::new(wasm_module_t::new(module))),
	Err(_) => None,
	}
	}

[alexcrichton]

Ah for that case specifically the panic is this call to .expect and that's not something you should be catching in the C API but is instead an error which should be bubbled up. If you'd like sending a PR there to use ? to bubble up the error I believe would fix that issue (as it'd be returned through the error returned by wasm_module_new)