Skip to content

Update implementation and tests for the trap redesign#175

Merged
peterhuene merged 9 commits into
bytecodealliance:mainfrom
kpreisser:update-tests-for-trap-redesign
Nov 10, 2022
Merged

Update implementation and tests for the trap redesign#175
peterhuene merged 9 commits into
bytecodealliance:mainfrom
kpreisser:update-tests-for-trap-redesign

Conversation

@kpreisser
Copy link
Copy Markdown
Contributor

@kpreisser kpreisser commented Nov 3, 2022

Hi, this updates the implementation and tests for the trap redesign (bytecodealliance/wasmtime#5149, bytecodealliance/wasmtime#5215).

Fixes #173

  • The ExitCode property is moved from TrapException/TrapAccessor to WasmtimeException, as the WASI exit code now needs to be retrieved from an wasmtime_error_t*.
  • The Frames property is moved from TrapException to WasmtimeException, since both errors and traps can have traces now.
  • The functionality of passing the original .NET exception as InnerException (see Pass original .NET callback exception as TrapException's inner exception  #172) is moved from the TrapException to the WasmtimeException, since host-created traps (with wasmtime_trap_new()) are now returned as error (wasmtime_error_t*).
    (While theoretically it could happen that the returned wasmtime_error_t* has actually another cause than the error returned at the host-to-wasm transition, I assume this won't be the case, similar as to a .NET exception that would just bubble up the stack once thrown.)

Currently, tests that are still failing are the following:

See bytecodealliance#173

Currently, tests that are still failing are the following:
- ExitTrapTests: It is not clear how to get the WASI exit code from a `wasmtime_error_t*` instead of an `wasmtime_trap_t*` via the C API.
- FuelConsumptionTests: `wasmtime_trap_code` unexpectedly fails (SEHException) even though a trap was returned.
- TrapTests: The tests introduced with bytecodealliance#172 fail as the inner exception would need to be set on the WasmtimeException instead of TrapException. Need to check how to solve this.
This moves the ExitStatus from TrapException/TrapAccessor to WasmtimeException.
@kpreisser kpreisser changed the title Update tests for the new trap behavior Update implementation and tests for the new trap behavior Nov 7, 2022
@kpreisser kpreisser changed the title Update implementation and tests for the new trap behavior Update implementation and tests for the trap redesign Nov 7, 2022
@kpreisser kpreisser marked this pull request as draft November 7, 2022 20:46
… retrieve the frames for a wasmtime_error_t* using the new wasmtime_error_wasm_trace function.

TODO: Add tests.
…y represent a pointer, but a platform-sized integer.

Note that nint/nuint in C# will behave like regular numeric types like int/uint, so e.g. the checked/unchecked context is respected, which is not the case with IntPtr/UIntPtr until .NET 7.
…rException from the TrapException to the WasmtimeException, since host-created traps (with wasm_trap_new) are now returned as error (wasmtime_error_t*).

While theoretically it could happen that the returned wasmtime_error_t* has actually another cause than the error returned at the host-to-wasm transition, I assume this is not the case, similar as to a .NET exception that would just bubble up the stack once thrown.
@peterhuene
Copy link
Copy Markdown
Member

peterhuene commented Nov 8, 2022

It appears that OutOfFuel trap code is missing from the C API, which is why the fuel tests I think are crashing.

I'll try to fix this upstream.

@peterhuene
Copy link
Copy Markdown
Member

Fix upstream at bytecodealliance/wasmtime#5230.

…trap.h header file.

See GHSA-h84q-m8rr-3v9q

Previously, we were passing a pointer to an Int32 (4-byte buffer), so the definition actually matched the Rust code, and therefore prior versions of wasmtime-dotnet are not impacted by that issue.
@kpreisser kpreisser marked this pull request as ready for review November 10, 2022 19:02
@kpreisser
Copy link
Copy Markdown
Contributor Author

kpreisser commented Nov 10, 2022

I added the OutOfFuel trap code and fixed the definition of wasmtime_trap_code (see GHSA-h84q-m8rr-3v9q) , so once bytecodealliance/wasmtime#5230 is merged, I think this should be complete 🙂

Note: Previously, wasm_trap_code in wasmtime-dotnet was defined to take a pointer to a int (32-bit) which actually matched the Rust code definition rather than the C header definition (trap.h), so prior versions of wasmtime-dotnet are not affected by the out-of-bounds write.

peterhuene
peterhuene previously approved these changes Nov 10, 2022
@peterhuene
Copy link
Copy Markdown
Member

Thanks! Now that it has merged, once the dev artifact is published with that commit, I'll restart CI for this PR and merge.

@kpreisser
Copy link
Copy Markdown
Contributor Author

Sorry, I forgot to update the exception messages in the FuelConsumptionTests (checked with a locally built Wasmtime).

peterhuene
peterhuene previously approved these changes Nov 10, 2022
@peterhuene
Copy link
Copy Markdown
Member

peterhuene commented Nov 10, 2022

It looks like the CI is now failing for a legitimate reason in the examples, as we're currently expecting a TrapException and not WasmtimeException.

@kpreisser
Copy link
Copy Markdown
Contributor Author

Sorry, I hadn't thought of checking the examples. Wasmtime now seems to return the not enough fuel remaining in store message as wasmtime_error_t* instead of wasm_trap_t*, so the example should probably be updated to catch a WasmtimeException, similar to the change in the FuelConsumptionTests:

Action action = () => Store.ConsumeFuel(2000UL);
action
.Should()
.Throw<WasmtimeException>()
.WithMessage("not enough fuel remaining in store*");

(Note however, that the all fuel consumed by WebAssembly message is still returned as wasm_trap_t*, with the new OutOfFuel code.)

I can take a look at this tomorrow, unless you beat me to it 🙂

@peterhuene
Copy link
Copy Markdown
Member

I'll push up the fix.

This commit updates the consume-fuel example to catch `WasmtimeException`
instead of `TrapException`.

The change is due to the `ConsumeFuel` call returning an error when it
fails to consume the given amount of fuel and that propagates properly now.
@peterhuene
Copy link
Copy Markdown
Member

Thanks again for fixing this!

@peterhuene peterhuene merged commit fe87f2f into bytecodealliance:main Nov 10, 2022
@kpreisser kpreisser deleted the update-tests-for-trap-redesign branch November 11, 2022 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI is red

2 participants