Skip to content

Add stack overflow checks in wasm backend? #9039

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

Closed
kripken opened this issue Jul 19, 2019 · 5 comments · Fixed by #9141 or #12095
Closed

Add stack overflow checks in wasm backend? #9039

kripken opened this issue Jul 19, 2019 · 5 comments · Fixed by #9141 or #12095

Comments

@kripken
Copy link
Member

kripken commented Jul 19, 2019

In fastcomp, ASSERTIONS=1 adds stack overflow checks. The wasm backend doesn't have this, but maybe it's useful and we should add it?

@VirtualTim
Copy link
Collaborator

My understanding is that the UBSan work @quantum5 has been doing is kind of a better version of this sort of stuff.
Maybe it's better to just use that instead? The only downside I can think of is that it currently doesn't work with threads.

@kripken
Copy link
Member Author

kripken commented Jul 22, 2019

Ah, good point @VirtualTim! Yes, I think UBSan or ASan should handle this - @quantum5 can you confirm? If so then I agree we should indeed just point people to that.

@quantum5
Copy link
Member

ASan is not designed to catch stack overflows, just like how it's not designed to catch null deferences. On native platforms, that is done by putting invalid pages. But I am sure ASan can be modified to do so on Emscripten, just like it has been modified to catch null deferences.

I think we can just overallocate, say 64 KB of stack, and poison the shadow memory for it.

The thread part will have to wait until #9076, of course.

@quantum5
Copy link
Member

ASan doesn't seem to work too reliably for this purpose.

I think a binaryen pass that ensures the stack pointer does not dip below the end of the stack is simpler and makes more sense.

@kripken
Copy link
Member Author

kripken commented Aug 30, 2020

The test for this for the wasm backend, test_stack_overflow_check, is still disabled. It looks like it can't be enabled, as the code so far just checks if the new value of the stack is above the lower limit. However, it is an unsigned comparison, so if we try to allocate so much that it wraps around into a huge amount, it looks higher than the limit, and passes the check, which fails the test.

@kripken kripken reopened this Aug 30, 2020
kripken added a commit to WebAssembly/binaryen that referenced this issue Sep 2, 2020
See emscripten-core/emscripten#9039 (comment)

The valid stack area is a region [A, B] in memory. Previously we just checked that
new stack positions S were S >= A, which prevented us from growing too much
(the stack grows down). But that only worked if the growth was small enough to not
overflow and become a big unsigned value. This PR makes us check the other way
too, which requires us to know where the stack starts out at.

This still supports the old way of just passing in the growth limit. We can remove it
after the roll.

In principle this can all be done on the LLVM side too after emscripten-core/emscripten#12057
but I'm not sure of the details there, and this is easy to fix here and get testing up
(which can help with later LLVM work).

This helps emscripten-core/emscripten#11860 by allowing us to clean up
some fastcomp-specific stuff in tests.
kripken added a commit that referenced this issue Sep 3, 2020
Depends on WebAssembly/binaryen#3091

This both enables the check, and adds a mode that checks many small
allocations instead of one huge one. That ensures we check both the lower
and upper limits of the stack region, and so are safe from the overflow
issue mentioned in #9039 (comment)

Also fix when the initialization happened - it was in callMain but that
didn't help cases without main, or using minimal runtime.

Also fix the skipping in test_core.py which looked for the old option.
With that fixed, wasm2ss (the test mode that runs with full stack
checks) all passes.

Fixes #9039 and re-enables `test_safe_stack_dylink` after the
binaryen roll + these fixes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants