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

Increase the size of the sigaltstack. #1298

Closed
wants to merge 1 commit into from
Closed

Conversation

sunfishcode
Copy link
Member

Rust's stack overflow handler installs a sigaltstack stack with size
SIGSTKSZ, which is too small for some of the things we do in signal
handlers. Install bigger sigaltstack stacks so that we have enough
space.

cc #900. The code here is a sketch of a possible
alternative. Advantages of this approach include not needing extra
prologue checks, not disabling the Rust stack overflow message, and
ensuring that we always have a guard page.

Rust's stack overflow handler installs a sigaltstack stack with size
SIGSTKSZ, which is too small for some of the things we do in signal
handlers. Install bigger sigaltstack stacks so that we have enough
space.
@sunfishcode sunfishcode added the wasmtime Issues about wasmtime that don't fall into another label label Mar 12, 2020
@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Mar 12, 2020
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Would it be possible to migrate this to Rust instead of C++? I'm hoping eventually to remove the need for C++ except for FFI definitions of setjmp/longjmp in the long term, and I don't think there's any fundamental reason why this needs to be in C++ rather than Rust.

Additionally could you add some comments about how libstd is configuring a sigaltstack, but it's too small for our purposes (w/ references to the two issues so far) so we make sure that whenever wasm is called we have a bigger sigaltstack.

if (mprotect(stack_ptr, sigaltstack_size, PROT_READ | PROT_WRITE) != 0 ||
sigaltstack(&new_stack, &old_stack) != 0 ||
old_stack.ss_flags != 0 ||
old_stack.ss_size > sigaltstack_size)
Copy link
Member

Choose a reason for hiding this comment

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

Could this test be done before we start mmap'ing? We could check to see if the sigaltstack size is already big enough or if it's disabled then we we can act before we mmap our own and set it.

sigaltstack(&new_stack, &old_stack) != 0 ||
old_stack.ss_flags != 0 ||
old_stack.ss_size > sigaltstack_size)
abort();
Copy link
Member

Choose a reason for hiding this comment

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

How come there's an OOM trap, but then this is an abort? Could this translate to a trap as well?

@alexcrichton
Copy link
Member

Also FWIW I see this as mostly a temporary patch. #900 is sort of fundamentaly un-fixable with our current strategy of stack overflow detection because we'll always have the case that wasm calls a ton of functions then calls some native code which aborts the whole process instead of returning a wasm trap. For example you could have something like:

(module 
  (import "" "" (func $f))
  (func $recurse
    call $f
    call $recurse)
  (start $recurse))

That would test calling a native function on every single wasm stack frame, and eventually the native code will abort the process due to stack overflow when in reality this is a wasm stack overflow condition.

This'll make it so stack overflow doesn't trigger a second stack overflow when generating a backtrace, but we'll still want to handle this recursive case.

alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Mar 12, 2020
This'll get fixed in bytecodealliance#1298 but for now let's get CI working again
alexcrichton added a commit that referenced this pull request Mar 12, 2020
This'll get fixed in #1298 but for now let's get CI working again
@github-actions
Copy link

Subscribe to Label Action

This issue or pull request has been labeled: "wasmtime", "wasmtime:api"

Users Subscribed to "wasmtime:api"

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@sunfishcode
Copy link
Member Author

Closing in favor of #1315.

abrown pushed a commit to abrown/wasmtime that referenced this pull request Mar 18, 2020
…ytecodealliance#1298)

This patch adds a third mode for templates: REX inference is requestable
at template instantiation time. This reduces the number of recipes
by removing rex()/nonrex() redundancy for many instructions.
@sunfishcode sunfishcode deleted the bigger-sigaltstack branch March 26, 2020 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime Issues about wasmtime that don't fall into another label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants