Define __stack_base and __stack_limit globals in debug mode for stack overflow detection

[loganek]

That enables VMs to implement stack overflow detection or using passes like https://github.com/WebAssembly/binaryen/blob/main/src/passes/StackCheck.cpp

See WebAssembly/wasi-threads#12

[sunfishcode]

I recognize that there are engines and tools implementing this already. But has there been any discussion of whether this is something we want to officially support in WASI contexts?

An alternative would be to add code to LLVM to perform stack overflow checks when it bumps the stack pointer. That would have the advantage of keeping the stack pointer and the stack a toolchain implementation detail, rather than exposing them as a toolchain/engine convention.

[loganek]

But has there been any discussion of whether this is something we want to officially support in WASI contexts?

No, there wasn't any discussion other than WebAssembly/wasi-threads#12 I'm happy to discuss it more though either here or in the linked issue.

An alternative would be to add code to LLVM to perform stack overflow checks when it bumps the stack pointer.

I don't think that's exclusive to proposed changes here? I'd assume the userspace code would have to somehow set stack boundaries (as it's the one that allocates memory for the stack) so LLVM can generate the code for stack overflow check (e.g. we could possibly integrate the pass from binaryen to LLVM). In that case, LLVM would have to define those globals, so we remove definitions of __stack_base/__stack_limit from this change.

Or did you have something completely different in mind?

[yamt]

• i feel it's simpler to make them arguments of wasi_thread_spawn
• i don't think it's a good idea to flip abi on NDEBUG

[loganek]

i feel it's simpler to make them arguments of wasi_thread_spawn

Yeah, that was the option I suggested WebAssembly/wasi-threads#12 but I didn't really like it so I asked others for opinion. The reason I'm not sure if making them arguments of wasi_thread_spawn is a good idea is because the concept of stack might not exist in some applications. I think there are cases where supporting a subset of applications is the right thing, although I'm not 100% sure that's the case for wasi_thread_spawn.

My use of stack overrun check is only in debug mode, but I guess others might use it in production code as well; another option would be to always check if globals __stack_base/__stack_limit exists, and if so, set it on thread start (similarly to what I described in the comment #380 (comment)). That will add a few extra instructions, but maybe that's not such a big deal? Then the tooling can generate those globals when a specific flag is enabled (--enable-stack-overrun-check ?) along with generating a code for checking the stack pointer.

[yamt]

I recognize that there are engines and tools implementing this already. But has there been any discussion of whether this is something we want to officially support in WASI contexts?

is WASI the right forum for this topic?
iirc, the stack pointer global is defined in C ABI, which is a bit broader than WASI.

[sunfishcode]

I don't think that's exclusive to proposed changes here? I'd assume the userspace code would have to somehow set stack boundaries (as it's the one that allocates memory for the stack) so LLVM can generate the code for stack overflow check (e.g. we could possibly integrate the pass from binaryen to LLVM). In that case, LLVM would have to define those globals, so we remove definitions of __stack_base/__stack_limit from this change.

Your top post here says that VMs may implement the stack check. That suggests that you're proposing a change to the module-engine ABI. Is that the plan here? If so, then I have the concern that this is adding things previously considered implementation details to the module-engine ABI, and I'd like to suggest doing the checking in LLVM or other tools instead.

If you're proposing that this is just part of the C ABI, and the LLVM or binaryen or other tools will insert the checking code into the wasm module, then this is just part of the C ABI, and it looks reasonable to me.

Doing the checking in the wasm module does increase code size, which I care about, so a possible way to optimize this would be to explore adding a feature to the core wasm spec to define value-range predicates for globals, looking something like this:

  (global $sp.base (mut i32) (i32.const 0))
  (global $sp (mut i32) (i32.const 0) (i32.ge_u (global.get $sp.base) (global.get $sp))
  (global $sp.limit (mut i32) (i32.const 0) (i32.ge_u (global.get $sp) (global.get $sp.limit))

Like global inits, there'd be rules about what form the expression can have. Just a simple i32.ge_u with another global like the above would go a long way.

[sbc100]

I don't think that's exclusive to proposed changes here? I'd assume the userspace code would have to somehow set stack boundaries (as it's the one that allocates memory for the stack) so LLVM can generate the code for stack overflow check (e.g. we could possibly integrate the pass from binaryen to LLVM). In that case, LLVM would have to define those globals, so we remove definitions of __stack_base/__stack_limit from this change.

Your top post here says that VMs may implement the stack check. That suggests that you're proposing a change to the module-engine ABI. Is that the plan here? If so, then I have the concern that this is adding things previously considered implementation details to the module-engine ABI, and I'd like to suggest doing the checking in LLVM or other tools instead.

If you're proposing that this is just part of the C ABI, and the LLVM or binaryen or other tools will insert the checking code into the wasm module, then this is just part of the C ABI, and it looks reasonable to me.

Doing the checking in the wasm module does increase code size, which I care about, so a possible way to optimize this would be to explore adding a feature to the core wasm spec to define value-range predicates for globals, looking something like this:

  (global $sp.base (mut i32) (i32.const 0))
  (global $sp (mut i32) (i32.const 0) (i32.ge_u (global.get $sp.base) (global.get $sp))
  (global $sp.limit (mut i32) (i32.const 0) (i32.ge_u (global.get $sp) (global.get $sp.limit))

Like global inits, there'd be rules about what form the expression can have. Just a simple i32.ge_u with another global like the above would go a long way.

Interesting idea! I wonder if there are any other uses for these kind of bounds checks?

I wonder if the engine could make this fast enough to warrant enabling these check in release build too? Presumably they won't come at zero cost.

Thus far (at least in emscripten) detecting shadow stack overflow has been something we only recommend in debug builds. Normally in debug builds folks tend to care less about that extra size (and performance) costs of the checks.

Even if we do go for a core wasm proposal to make this more efficient, there are good reasons to want to use the user-space / in-module checks until that becomes available. It makes sense to me that the in-module checks should be part of the C ABI / tooling conventions.

[yamt]

I don't think that's exclusive to proposed changes here? I'd assume the userspace code would have to somehow set stack boundaries (as it's the one that allocates memory for the stack) so LLVM can generate the code for stack overflow check (e.g. we could possibly integrate the pass from binaryen to LLVM). In that case, LLVM would have to define those globals, so we remove definitions of __stack_base/__stack_limit from this change.

Your top post here says that VMs may implement the stack check. That suggests that you're proposing a change to the module-engine ABI. Is that the plan here? If so, then I have the concern that this is adding things previously considered implementation details to the module-engine ABI, and I'd like to suggest doing the checking in LLVM or other tools instead.

WAMR has a heuristic to detect C ABI and perform the stack overflow/underflow checks. (it isn't new.)
it doesn't work for wasi-threads' libc-allocated stacks though.
i think it's one of the motivations of this proposal.

[loganek]

Hi,
sorry for late reply.

Your top post here says that VMs may implement the stack check. That suggests that you're proposing a change to the module-engine ABI. Is that the plan here?

No, my plan was to extend the C ABI (I'll update the relevant documents in the tooling convention's repo once we agree on the approach here). I mentioned VMs because this is what WAMR currently does for single-threaded applications (as @yamt said, it has ways to figure out the stack pointer, so it could potentially also read the stack boundaries in multi-threaded app, at least until the tooling is ready).

Doing the checking in the wasm module does increase code size, which I care about, so a possible way to optimize this would be to explore adding a feature to the core wasm spec to define value-range predicates for globals, looking something like this:

That's interesting idea. I'll think about it and might discuss that with others. Do you think that's a blocker though for proceeding with extending C ABI? I personally only need that feature in debug mode, however, once this becomes part the C ABI, I guess we'll have to enable it for release builds as well (unless we specify in the doc that it's an optional feature).

[sunfishcode]

That's interesting idea. I'll think about it and might discuss that with others. Do you think that's a blocker though for proceeding with extending C ABI? I personally only need that feature in debug mode, however, once this becomes part the C ABI, I guess we'll have to enable it for release builds as well (unless we specify in the doc that it's an optional feature).

No, I don't think it's a blocker.

Since this is about the C ABI, it's not specific to WASI, so addressing @yamt's question above, I think that means the place to propose this is the C ABI, or possibly a new document in the tool-conventions repository.