Stream: git-wasmtime

Topic: wasmtime / PR #8301 cranelift: Include clobbers and outgo...


view this post on Zulip Wasmtime GitHub notifications bot (Apr 04 2024 at 22:39):

jameysharp opened PR #8301 from jameysharp:fix-stack-limit to bytecodealliance:main:

When we compute the amount of space that we need in a stack frame for the stack limit check, we were only counting spill-slots and explicit stack-slots. However, we need to account for all uses of the stack which occur before the next stack limit check. That includes clobbers and any stack arguments we want to pass to callees.

The maximum amount that we could have missed by is essentially bounded by the number of arguments which could be passed to a function. In Wasmtime, that is limited by MAX_WASM_FUNCTION_PARAMS in wasmparser::limits, which is set to 1,000, and the largest arguments are 16-byte vectors, so this could undercount by about 16kB.

This is not a security issue according to Wasmtime's security policy (https://docs.wasmtime.dev/security-what-is-considered-a-security-vulnerability.html) because it's the embedder's responsibility to ensure that the stack where Wasmtime is running has enough extra space on top of the configured max_wasm_stack size, and getting within 16kB of the host stack size is too small to be safe even with this fixed.

However, this was definitely not the intended behavior when stack limit checks or stack probes are enabled, and anyone with non-default configurations or non-Wasmtime uses of Cranelift should evaluate whether this bug impacts your use case.

(For reference: When Wasmtime is used in async mode or on Linux, the default stack size is 1.5MB larger than the default WebAssembly stack limit, so such configurations are typically safe regardless. On the other hand, on macOS the default non-async stack size for threads other than the main thread is the same size as the default for max_wasm_stack, so that is too small with or without this bug fix.)

cc: @uweigand @bjorn3 @afonso360

view this post on Zulip Wasmtime GitHub notifications bot (Apr 04 2024 at 22:39):

jameysharp requested wasmtime-compiler-reviewers for a review on PR #8301.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 04 2024 at 22:39):

jameysharp requested fitzgen for a review on PR #8301.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 04 2024 at 22:53):

jameysharp requested wasmtime-core-reviewers for a review on PR #8301.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 04 2024 at 22:53):

jameysharp updated PR #8301.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 05 2024 at 09:11):

uweigand commented on PR #8301:

This makes sense to me, yes.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 05 2024 at 15:35):

fitzgen submitted PR review:

LGTM!

view this post on Zulip Wasmtime GitHub notifications bot (Apr 05 2024 at 16:32):

jameysharp merged PR #8301.


Last updated: Dec 23 2024 at 12:05 UTC