Skip to content

Add -s SAFE_STACK to detect stack overflow #9141

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

Merged
merged 7 commits into from
Aug 6, 2019

Conversation

quantum5
Copy link
Member

@quantum5 quantum5 commented Aug 2, 2019

Depends on WebAssembly/binaryen#2278 (tests will not pass without it).

Also added a test mode wasm2ss to run the whole test suite under -s SAFE_STACK. It seems to work.

Fixes #9039.

@quantum5 quantum5 requested review from kripken, sbc100 and tlively August 2, 2019 00:05
@juj
Copy link
Collaborator

juj commented Aug 2, 2019

Why have a separate setting SAFE_STACK and not reuse the existing STACK_OVERFLOW_CHECK directive?

@@ -5162,6 +5162,9 @@ LibraryManager.library = {
},
// =======================================================================

__handle_stack_overflow: function() {
Copy link
Collaborator

@juj juj Aug 2, 2019

Choose a reason for hiding this comment

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

__handle_stack_overflow -> __abort_stack_overflow?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if __abort_stack_overflow is really a good name for the handler function. Binaryen gives the runtime flexibility to do whatever in face of stack overflow. Hypothetically, a runtime could implement a mechanism similar to sigaltstack(2) to handle stack overflows.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, now that I look closer at this - does the wasm backend directly generate a call to this function? If so, then the name is very generic. How about __wasm_stack_overflow? Having compiler backend construct a function call with such a name that does not connect to an established API would make the symbol look somewhat floating.

Copy link
Member Author

Choose a reason for hiding this comment

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

The WASM backend does not generate a call to this. wasm-emscripten-finalize in binaryen inserts a call to this function when passed a certain flag. So the __wasm prefix is inappropriate.

@quantum5
Copy link
Member Author

quantum5 commented Aug 2, 2019

Why have a separate setting SAFE_STACK and not reuse the existing STACK_OVERFLOW_CHECK directive?

It doesn't do the same thing. STACK_OVERFLOW_CHECK adds security cookies and checks them. SAFE_STACK instruments every stack access and make sure it doesn't dip below the end of the stack.

@quantum5 quantum5 marked this pull request as ready for review August 3, 2019 00:36
@quantum5 quantum5 merged commit 14ff5bd into emscripten-core:incoming Aug 6, 2019
@@ -242,6 +242,10 @@ var SAFE_HEAP = 0;
// Log out all SAFE_HEAP operations
var SAFE_HEAP_LOG = 0;

// Check each stack pointer decrement on WASM backend to ensure that the stack
Copy link
Member

Choose a reason for hiding this comment

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

nit (not worth a PR): wasm isn't an acronym, so just "wasm" is fine.

belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
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.

Add stack overflow checks in wasm backend?
4 participants