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

Enable limited stack overflow checks while running inside continuations #136

Merged
merged 4 commits into from
Mar 21, 2024

Conversation

frank-emrich
Copy link

Currently, we can overflow the stack while running inside a continuation, without the runtime having any way of detecting this.
This PR partially rectifies this, by making the existing stack limit checks that get emitted by cranelift in every wasm function prelude work correctly while running inside a continuation.

All that was required to enable the stack limit checks was the following:

  1. Stop zero-ing out the stack_limit value in VMRuntimeLimits whenever we resume a continuation.
  2. When creating a continuation, set a reasonable value for the stack_limits value in its StackLimits object.

Note that all the required infrastructure to make sure that whenever we switch stacks, we save and restore the stack_limits value inside VMRuntimeLimits and the StackLimits object of the involved stacks was already implemented in #98 and #99. In this sense, enabling these checks is "free": The limits were already checked, but previously using a limit of 0.

The only remaining question is what the "reasonable value" for the stack limits value mentioned above is. As discussed in #122, the stack limit checks that cranelift emits in function preludes are rather limited, and these limitations are reflected in the checks that this PR provides:
When entering a wasm function, they check that the current stack pointer is larger than the stack_limit value in VMRuntimeLimits. They do not take into account how much stack space the function itself will occupy. No stack limit checks are performed when calling a host function.

Thus, this PR defines a config option wasmfx_red_zone_size. The idea is that we define the stack limit as bottom_of_fiber_stack + wasmfx_red_zone_size. Thus, the stack checks boil down to the following:
Whenever we enter a wasm function while inside a continuation, we ensure that there are at least wasmfx_red_zone_size bytes of stack space left.

I've set the default value for wasmfx_red_zone_size to 32k. To get a rough idea for a sensible value, I determined that a call to the fd_write WASI function occupies ~21k of stack space, and generously rounded this up to 32k.

Important: This means that these stack limit checks are incomplete: Calling a wasm or host function that occupies more than wasmfx_red_zone_size of stack space may still result in an undetected stack overflow!

@frank-emrich frank-emrich requested a review from dhil March 20, 2024 15:23
Copy link
Member

@dhil dhil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Comment on lines 214 to 216
/// Space that must be left on stack when starting execution of a
/// function while running on a continuation stack.
pub wasmfx_red_zone_size: Option<usize>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

presumably this is platform/abi dependent? Like SysV mandates 16 bytes alignment. I suppose we round up when actually allocate the stack. Nonetheless, it may be worth writing here that the size given here is an underapproximation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed elsewhere, this value does not influence the allocation of stacks; it isn't really constrained at all in that regard. The only condition is that it should be smaller than the stack size. I've added a comment stating this condition, even though we don't currently check it.

@frank-emrich frank-emrich enabled auto-merge (squash) March 21, 2024 13:21
@frank-emrich frank-emrich merged commit d6522e2 into wasmfx:main Mar 21, 2024
17 checks passed
@frank-emrich frank-emrich deleted the simple-stack-limit-check branch March 21, 2024 14:03
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.

2 participants