Update definition of wasmtime_func_call_unchecked.#245
Merged
peterhuene merged 2 commits intobytecodealliance:mainfrom May 3, 2023
Merged
Update definition of wasmtime_func_call_unchecked.#245peterhuene merged 2 commits intobytecodealliance:mainfrom
wasmtime_func_call_unchecked.#245peterhuene merged 2 commits intobytecodealliance:mainfrom
Conversation
Contributor
Author
|
I can consistently reproduce the panic using the following C# console program on Windows 10 Version 21H2 x64 (using .NET 7.0.5), even without attaching a debugger (and I assume, but haven't tested, that it would also occur on Linux, as it also occured there on CI): using Wasmtime;
using var engine = new Engine();
using var module = Module.FromText(engine, "Error", @"
(module
(import """" ""host_start"" (func $host_start))
(import """" ""host_get_trap"" (func $host_get_trap))
(export ""host_get_trap"" (func $host_get_trap))
(start $start)
(func $start
(call $host_start)
)
)
");
using var store = new Store(engine);
using var linker = new Linker(engine);
linker.Define("", "host_start", Function.FromCallback(
store,
(caller, args, results) => { },
Array.Empty<ValueKind>(),
Array.Empty<ValueKind>()));
linker.Define("", "host_get_trap", Function.FromCallback(
store,
(caller, args, results) => throw new Exception("This is an expected exception."),
Array.Empty<ValueKind>(),
Array.Empty<ValueKind>()));
var instance = linker.Instantiate(store, module);
var getTrap = instance.GetFunction("host_get_trap")!;
// This stackalloc seems necessary to trigger the trap at least
// when using wasmtime_func_call_unchecked.
Span<byte> testStack = stackalloc byte[8192];
try
{
getTrap.Invoke();
}
catch (WasmtimeException ex)
{
Console.WriteLine(ex.ToString());
}Note that this uses When using Bisecting shows that this issue appears to have been introduced with commit bytecodealliance/wasmtime@913efdf (bytecodealliance/wasmtime#6262); when using the previous commit bytecodealliance/wasmtime@5b9121f, the output is as expected: |
Member
|
I'll see if I can make a pure |
Member
|
I've managed to reproduce with this: #[test]
fn regression() -> Result<()> {
let engine = Engine::default();
let module = Module::new(
&engine,
r#"
(module
(import "" "host_start" (func $host_start))
(import "" "host_get_trap" (func $host_get_trap))
(export "get_trap" (func $host_get_trap))
(start $start)
(func $start
(call $host_start)
)
)
"#,
)?;
let mut store = Store::new(&engine, ());
let mut linker = Linker::new(&engine);
let host_start = Func::new(
&mut store,
FuncType::new([], []),
|_caller, _args, _results| Ok(()),
);
linker.define(&store, "", "host_start", host_start)?;
let host_get_trap = Func::new(
&mut store,
FuncType::new([], []),
|_caller, _args, _results| Err(anyhow::anyhow!("trap!!!")),
);
linker.define(&store, "", "host_get_trap", host_get_trap)?;
let instance = linker.instantiate(&mut store, &module)?;
let get_trap = instance.get_func(&mut store, "get_trap").unwrap();
let err = get_trap.call(&mut store, &[], &mut []).unwrap_err();
assert!(err.to_string().contains("trap!!!"));
Ok(())
}Thanks for filing an issue and providing a test case! |
fitzgen
added a commit
to fitzgen/wasmtime
that referenced
this pull request
May 1, 2023
… Wasm Fixes a regression from bytecodealliance#6262, originally reported in bytecodealliance/wasmtime-dotnet#245 The issue was that we would enter Wasm and save the stack-walking registers but never clear them after Wasm returns. Then if a host-to-host call tried to capture a stack, we would mistakenly attempt to use those stale registers to start the stack walk. This mistake would be caught by an assertion, triggering a panic. This commit fixes the issue by managing the save/restore in the `CallThreadState` construction/drop, rather than in the old `set_prev` method. Co-Authored-By: alex@alexcrichton.com
fitzgen
added a commit
to fitzgen/wasmtime
that referenced
this pull request
May 1, 2023
… Wasm Fixes a regression from bytecodealliance#6262, originally reported in bytecodealliance/wasmtime-dotnet#245 The issue was that we would enter Wasm and save the stack-walking registers but never clear them after Wasm returns. Then if a host-to-host call tried to capture a stack, we would mistakenly attempt to use those stale registers to start the stack walk. This mistake would be caught by an assertion, triggering a panic. This commit fixes the issue by managing the save/restore in the `CallThreadState` construction/drop, rather than in the old `set_prev` method. Co-Authored-By: Alex Crichton <alex@alexcrichton.com>
fitzgen
added a commit
to fitzgen/wasmtime
that referenced
this pull request
May 2, 2023
… Wasm Fixes a regression from bytecodealliance#6262, originally reported in bytecodealliance/wasmtime-dotnet#245 The issue was that we would enter Wasm and save the stack-walking registers but never clear them after Wasm returns. Then if a host-to-host call tried to capture a stack, we would mistakenly attempt to use those stale registers to start the stack walk. This mistake would be caught by an assertion, triggering a panic. This commit fixes the issue by managing the save/restore in the `CallThreadState` construction/drop, rather than in the old `set_prev` method. Co-Authored-By: Alex Crichton <alex@alexcrichton.com>
github-merge-queue bot
pushed a commit
to bytecodealliance/wasmtime
that referenced
this pull request
May 2, 2023
… Wasm (#6321) Fixes a regression from #6262, originally reported in bytecodealliance/wasmtime-dotnet#245 The issue was that we would enter Wasm and save the stack-walking registers but never clear them after Wasm returns. Then if a host-to-host call tried to capture a stack, we would mistakenly attempt to use those stale registers to start the stack walk. This mistake would be caught by an assertion, triggering a panic. This commit fixes the issue by managing the save/restore in the `CallThreadState` construction/drop, rather than in the old `set_prev` method. Co-authored-by: Alex Crichton <alex@alexcrichton.com>
fitzgen
added a commit
to fitzgen/wasmtime
that referenced
this pull request
May 2, 2023
… Wasm Fixes a regression from bytecodealliance#6262, originally reported in bytecodealliance/wasmtime-dotnet#245 The issue was that we would enter Wasm and save the stack-walking registers but never clear them after Wasm returns. Then if a host-to-host call tried to capture a stack, we would mistakenly attempt to use those stale registers to start the stack walk. This mistake would be caught by an assertion, triggering a panic. This commit fixes the issue by managing the save/restore in the `CallThreadState` construction/drop, rather than in the old `set_prev` method. Co-Authored-By: Alex Crichton <alex@alexcrichton.com>
github-merge-queue bot
pushed a commit
to bytecodealliance/wasmtime
that referenced
this pull request
May 3, 2023
…ng Wasm (#6321) * wasmtime: Fix resetting stack-walking registers when entering/exiting Wasm Fixes a regression from #6262, originally reported in bytecodealliance/wasmtime-dotnet#245 The issue was that we would enter Wasm and save the stack-walking registers but never clear them after Wasm returns. Then if a host-to-host call tried to capture a stack, we would mistakenly attempt to use those stale registers to start the stack walk. This mistake would be caught by an assertion, triggering a panic. This commit fixes the issue by managing the save/restore in the `CallThreadState` construction/drop, rather than in the old `set_prev` method. Co-Authored-By: Alex Crichton <alex@alexcrichton.com> * Plumb through `VMRuntimeLimits` when capturing stack traces This way we can differentiate between the same module loaded in different stores and avoid leaking other stores' frames into our backtraces. Co-Authored-By: Jamey Sharp <jsharp@fastly.com> --------- Co-authored-by: Alex Crichton <alex@alexcrichton.com> Co-authored-by: Jamey Sharp <jsharp@fastly.com>
Contributor
Author
|
Thanks for fixing the issue @fitzgen! The unit tests now run successfully (also on my machine). |
peterhuene
approved these changes
May 3, 2023
Member
|
@kpreisser thanks for fixing this on the .NET SDK side! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Update definition of
wasmtime_func_call_unchecked. See bytecodealliance/wasmtime#6262Fixes #244
Note: The unit tests run successfully on my machine (Windows 10 x64) e.g. with
dotnet testor when running them in Visual Studio without debugging; but when I run them in Visual Studio with debugging, the testWasmtime.Tests.ErrorTests.ItPassesCallbackErrorCauseAsInnerExceptionalways fails with aSEHException, which seems to be caused by a Rust panic (using Wasmtime from commit b453c70d7af6db1550fd7bc17e2e69c88e89323b):I'm not sure what could be causing this, and why it happens only when running the tests with debugging.