wasmtime / PR #1315 Increase the size of the sigaltstack. · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #1315 Increase the size of the sigaltstack.

Wasmtime GitHub notifications bot (Mar 13 2020 at 21:03):

sunfishcode opened PR #1315 from bigger-sigaltstack-rust to master:

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, and as of this writing lacks a guard page. Install bigger
sigaltstack stacks so that we have enough space, and have a guard page.

This is similar to https://github.com/bytecodealliance/wasmtime/pull/1298 but rewritten in Rust

Wasmtime GitHub notifications bot (Mar 13 2020 at 21:22):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 13 2020 at 21:22):

alexcrichton created PR Review Comment:

FWIW panicking in destructors is pretty sketchy, and panicking in a TLS destructor is sort of even sketchier (almost guaranteed to abort the process). Perhaps these could switch to debug_assert statements?

Wasmtime GitHub notifications bot (Mar 13 2020 at 21:22):

alexcrichton created PR Review Comment:

Since Tls is constructed with a null ss_sp before we call mmap, I think this'll want to check for null and bail out because mmap would have failed.

Wasmtime GitHub notifications bot (Mar 13 2020 at 21:22):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 13 2020 at 21:22):

alexcrichton created PR Review Comment:

Instead of raising a trap could this perhaps bubble up some sort of error to the caller? I don't think the raising mechanism is needed here if this is reorganized a bit, for example the caller could do:
#[cfg(unix)]
unix::setup_sigaltstack()?;
and internally this module would check the TLS variable, and if it's not initialized would perform inline initialization which would bubble this error up through returns rather than a longjmp

Wasmtime GitHub notifications bot (Mar 13 2020 at 21:22):

alexcrichton created PR Review Comment:

I might recommend something like:

thread_local!(static TLS: RefCell<Tls> = RefCell::new(Tls::None);

enum Tls {
    None,
    Allocated(Sigaltstack), // `Drop for Sigaltstack` frees stuff
    CheckedAlready), // sigaltstack was previously big enough
}

Wasmtime GitHub notifications bot (Mar 13 2020 at 21:22):

alexcrichton created PR Review Comment:

FWIW this may be a bit easier with let mut old_stack = mem::zeroed() to make it a bit more portable if that ever comes up

Wasmtime GitHub notifications bot (Mar 13 2020 at 21:22):

alexcrichton created PR Review Comment:

I think these checks may want to move up to the front actually and not be assertions but rather runtime checks. With the C embedding we're not actually guaranteed the sigaltstack is set up (which I'm just now realizing is probably bad) because the Rust main function didn't happen. Additionally the application may have its own sigaltstack handled somewhere else.

I think we'll want to check the current sigaltstack here and if it's big enough just skip all this entirely, and then for flags I'm not really sure we need to check too much, but I don't think we should panic if the embedder has a particular configuration.

Wasmtime GitHub notifications bot (Mar 13 2020 at 21:26):

iximeow submitted PR Review.

Wasmtime GitHub notifications bot (Mar 13 2020 at 21:26):

iximeow created PR Review Comment:

just a note, the default signal stack should have a guard page soon: https://github.com/rust-lang/rust/pull/69969

Wasmtime GitHub notifications bot (Mar 25 2020 at 10:22):

eust-dfinity submitted PR Review.

Wasmtime GitHub notifications bot (Mar 25 2020 at 10:22):

eust-dfinity created PR Review Comment:

FYI in Dfinity we use std::cmp::max(SIGSTKSZ, 24 * 1024) here, because SIGSTKSZ on MacOS is already huge (128K) and reserving 512K per thread for the signal stack seemed like too much

Wasmtime GitHub notifications bot (Apr 03 2020 at 18:51):

alexcrichton updated PR #1315 from bigger-sigaltstack-rust to master:

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, and as of this writing lacks a guard page. Install bigger
sigaltstack stacks so that we have enough space, and have a guard page.

This is similar to https://github.com/bytecodealliance/wasmtime/pull/1298 but rewritten in Rust

Wasmtime GitHub notifications bot (Apr 03 2020 at 18:52):

alexcrichton updated PR #1315 from bigger-sigaltstack-rust to master:

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, and as of this writing lacks a guard page. Install bigger
sigaltstack stacks so that we have enough space, and have a guard page.

This is similar to https://github.com/bytecodealliance/wasmtime/pull/1298 but rewritten in Rust

Wasmtime GitHub notifications bot (Apr 03 2020 at 19:05):

alexcrichton updated PR #1315 from bigger-sigaltstack-rust to master:

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, and as of this writing lacks a guard page. Install bigger
sigaltstack stacks so that we have enough space, and have a guard page.

This is similar to https://github.com/bytecodealliance/wasmtime/pull/1298 but rewritten in Rust

Wasmtime GitHub notifications bot (Apr 03 2020 at 20:42):

sunfishcode created PR Review Comment:

s/reigster/register/

Wasmtime GitHub notifications bot (Apr 03 2020 at 20:42):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 03 2020 at 20:42):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 03 2020 at 21:36):

alexcrichton updated PR #1315 from bigger-sigaltstack-rust to master:

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, and as of this writing lacks a guard page. Install bigger
sigaltstack stacks so that we have enough space, and have a guard page.

This is similar to https://github.com/bytecodealliance/wasmtime/pull/1298 but rewritten in Rust

Wasmtime GitHub notifications bot (Apr 03 2020 at 21:36):

alexcrichton merged PR #1315.

Last updated: Nov 22 2024 at 16:03 UTC