Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Undefined behavior when callback exception handler throws another exception #187

Closed
kpreisser opened this issue Nov 19, 2022 · 0 comments · Fixed by #188
Closed

Undefined behavior when callback exception handler throws another exception #187

kpreisser opened this issue Nov 19, 2022 · 0 comments · Fixed by #188

Comments

@kpreisser
Copy link
Contributor

kpreisser commented Nov 19, 2022

Currently, function callbacks (at the wasm-to-host-transition) are expected to catch all .NET Exceptions and transform them into a wasm_trap_t* which is then returned (at the native-to-managed transition).
However, if the exception handler throws another exception (e.g. an OutOfMemoryException when allocating the byte array for the UTF-8 message fails, which can happen when the system is low on memory), the .NET runtime on Windows unwinds the stack up to the next .NET exception handler, which seems to be incompatible with Wasmtime and can cause undefined behavior.

Consider the following code (on Windows 10 x64, using commit 415438c), which provokes an ArgumentException (in place of an OutOfMemoryException) in the callback exception handler by creating a string that needs more than 2**31-1 bytes for the UTF-8 encoding:

using var engine = new Engine();
using var module = Module.FromText(
    engine,
    "hello",
    @"
(module
    (func $hello (import """" ""hello"") (param externref))
    (export ""hello"" (func $hello))
)");

using var linker = new Linker(engine);
using var store = new Store(engine);
    
linker.DefineFunction(
    "",
    "hello",
    (object? o) =>
    {
        // Create an exception message that will exceed the max byte array length
        // when trying to convert it to UTF-8.
        string exceptionMsg = new string('\u2192', 715827883);
        throw new InvalidOperationException(exceptionMsg);
    });

var instance = linker.Instantiate(store, module);
var hello = instance.GetAction<object>("hello")!;
try
{
    hello(new object());
}
catch (Exception ex)
{
    Console.WriteLine(ex.ToString());
}

store.GC();

Expected behavior:
A consistent behavior, e.g. the program always completes successfully, or always fails due to the same reason, without showing symptoms of undefined behavior like access violations or failed assertions.

Actual behavior (on Windows):
Most of the time, store.GC() throws an AccessViolationException, and sometimes it throws an SEHException, with an output like this:

thread '<unnamed>' panicked at '0x11ce59d0000 >= 0x11ce7460018', crates\runtime\src\traphandlers\backtrace.rs:228:9

This indicates that the application already has entered an undefined state prior to that point, which could cause security implications.

I'm not sure about how exactly the stack unwinding works with the .NET CLR, but on Windows it appears to unwind the stack for a .NET exception up to the next .NET exception handler, even if there are managed-to-native and native-to-managed transitions within these points. This seems to be incompatible with what Wasmtime expects from a function callback, as shown by the later access violation.

Therefore, I think we need to prevent a .NET exception from falling through the native-to-managed transition, e.g. by terminating the application with Environment.FailFast() in such a case.

Note: On Linux (tested on Ubuntu), the .NET CLR doesn't appear to be able to unwind the stack beyond the last native-to-managed transition, so there the application already crashes consistently when the exception occurs in the exception handler (I guess for the same reasons as described in bytecodealliance/wasmtime#1845 - notice that only the managed frames on top of the native-to-managed transition are shown in the exception stack trace):

Unhandled exception. System.ArgumentException: Conversion buffer overflow.
   at System.Text.UTF8Encoding.GetByteCount(String chars)
   at System.Text.Encoding.GetBytes(String s)
   at System.Text.UTF8Encoding.UTF8EncodingSealed.GetBytes(String s)
   at Wasmtime.Function.InvokeCallback(Delegate callback, MethodInfo callbackInvokeMethod, Caller caller, Boolean passCaller, Value* args, Int32 nargs, Value* results, Int32 nresults, IReadOnlyList`1 resultKinds, Boolean returnsTuple)
   at Wasmtime.Linker.<>c__DisplayClass48_0.<DefineFunction>b__0(IntPtr env, IntPtr callerPtr, Value* args, UIntPtr nargs, Value* results, UIntPtr nresults)

Thank you!

kpreisser added a commit to kpreisser/wasmtime-dotnet that referenced this issue Nov 19, 2022
…r exception, to ensure we don't let .NET exceptions fall through the native-to-managed transition.

Fixes bytecodealliance#187
@kpreisser kpreisser changed the title Undefined state when callback exception handler throws another (.NET) exception Undefined state when callback exception handler throws another exception Nov 19, 2022
@kpreisser kpreisser changed the title Undefined state when callback exception handler throws another exception Undefined behavior when callback exception handler throws another exception Nov 20, 2022
peterhuene pushed a commit that referenced this issue Nov 28, 2022
…r exception (#188)

* Terminate the process if the callback exception handler throws another exception, to ensure we don't let .NET exceptions fall through the native-to-managed transition.

Fixes #187

* Follow-Up: Update comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant