Bug 165546

We need to test that stack exhaustion can be caught as well.

<rdar://problem/29760307>

It looks like wasmCallingConvention::setupFrameInPrologue also needs a stack check. We could use vm().addressOfSoftStackLimit() but then that prevents us from making the code PIC as in bug #168264 (making it patchable means workers get their own code copy).

I think we'll need to write a fuzz test for this, because we want to test exhaustion for different types of frames, and a hard-coded test won't be able to hit all possible locations reliably. We should randomly generate functions which will have different stack size, and probabilistically the tests will hit different check points. A trap would mean we've missed one.

A TLS load for every function is not cool though. Maybe we can have some clever stack limit bit pattern?

Or not, Fit says TLS is cheap.

Working on this now

Created attachment 310205 [details] WIP

Created attachment 310210 [details] WIP Still need to do stack checks for the JS call ICs. I'm thinking that I'll just make the wasm caller of the JS call IC stub know if it makes any JS calls, and if it does, it can calculate the maximum number of arguments for all JS calls inside of it, and then we can incorporate that into the stack check. This will prevent each JS call stub from having to do its own stack check

Created attachment 310211 [details] WIP

Created attachment 310214 [details] WIP close to being done. I need to write some tests.

Attachment 310214 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:427: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 310335 [details] WIP Waiting on: https://bugs.webkit.org/show_bug.cgi?id=172188

Attachment 310335 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WasmBinding.cpp:42: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/wasm/WasmBinding.cpp:634: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/wasm/WasmBinding.cpp:637: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:430: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 4 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 310335 [details] WIP Attachment 310335 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/3755013 New failing tests: wasm.yaml/wasm/function-tests/factorial.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/factorial.js.default-wasm wasm.yaml/wasm/function-tests/factorial.js.wasm-no-cjit wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-no-cjit wasm.yaml/wasm/function-tests/float-sub.js.wasm-no-call-ic wasm.yaml/wasm/js-api/wasm-to-wasm.js.wasm-no-cjit wasm.yaml/wasm/js-api/wasm-to-wasm.js.default-wasm wasm.yaml/wasm/js-api/wasm-to-wasm.js.wasm-no-call-ic wasm.yaml/wasm/js-api/wasm-to-wasm.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/float-sub.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/stack-overflow.js.default-wasm wasm.yaml/wasm/function-tests/factorial.js.wasm-no-call-ic wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-no-call-ic wasm.yaml/wasm/function-tests/float-sub.js.wasm-no-cjit wasm.yaml/wasm/function-tests/float-sub.js.default-wasm

Comment on attachment 310335 [details] WIP Attachment 310335 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3761709 New failing tests: imported/w3c/web-platform-tests/css/selectors4/focus-display-none-001.html imported/w3c/web-platform-tests/css/selectors4/focus-within-display-none-001.html

Created attachment 310378 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Created attachment 310474 [details] patch

Attachment 310474 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:431: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 310475 [details] patch

Created attachment 310477 [details] patch

Attachment 310477 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:431: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 310477 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=310477&action=review Some features call C++ code, like grow_memory. Should these also be marked as m_makesCalls? > JSTests/wasm/function-tests/factorial.js:36 > +assert.throws(() => fac(10000000000000), RangeError, "Maximum call stack size exceeded."); 10,000,000,000,000 way exceeds i32. Can you use 1e9 or something smaller? > JSTests/wasm/function-tests/stack-overflow.js:39 > + // FIXME: asssert some stack size Bug # > JSTests/wasm/function-tests/stack-overflow.js:216 > + test(42); So there are 3 places we want to check for stack overflow: * Same instance wasm -> wasm direct call. * wasm -> wasm indirect call (same or different instance) * Call of any import (JS or wasm) where the stub is the thing that overflows That's tricky because making sure each is tripped is a flaky thing. I'm not sure this test hits all of these? I'd think that you want to test their combination, find the "limit" and then progressively back down, forcing each to fail by trying to cause just that type of call close to the edge of failure... That's kinda hard though! > Source/JavaScriptCore/ChangeLog:11 > + runtime option to configure this. TLS or wasm context (which can sometimes be TLS)? > Source/JavaScriptCore/ChangeLog:25 > + Surprisingly, this patch was neutral on WasmBench and TitzerBench. 🎉 > Source/JavaScriptCore/runtime/Options.h:459 > + v(bool, useFastTLSForWasmStackChecks, true, Normal, "If true (and fast TLS is enabled), we will store stack bounds in fast TLS. If false, we will store it on the context.") \ Yeah I thought it would always be a slot on the wasm context instead. Is there a perf difference for either being false? > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:411 > + // 2. Try to speed things up by skipping stack checks. Does this work for leaf JS -> wasm? > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:415 > + // stack that such a stub would use. Nice! > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:419 > + RELEASE_ASSERT(checkSize >= 0); Signed overflow is UB, so this check is technically dead. Should be a checked add instead (and the unsigned -> signed coercion could also fail... Not UB, impl-def instead).

Comment on attachment 310477 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=310477&action=review >> Source/JavaScriptCore/ChangeLog:11 >> + This patch adds stack checks to wasm. It implements it by storing the stack >> + bounds on the Context or in TLS. By default, we use TLS. However, there is a >> + runtime option to configure this. > > TLS or wasm context (which can sometimes be TLS)? Is there an observable difference in performance between these? If not, I'm not sure it's worth the use of our few fast TLS slots. > Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:148 > + wasmContext->setCachedStackLimit(bitwise_cast<void*>(std::numeric_limits<uintptr_t>::max())); Why is this correct? What happens if prevWasmContext == wasmContext? Wouldn't this blow away the previous cached stack bounds? If so, can you add a test?

Comment on attachment 310477 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=310477&action=review >> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:148 >> + wasmContext->setCachedStackLimit(bitwise_cast<void*>(std::numeric_limits<uintptr_t>::max())); > > Why is this correct? What happens if prevWasmContext == wasmContext? Wouldn't this blow away the previous cached stack bounds? > > If so, can you add a test? This is definitely not correct for the reason you outlined. I added it while testing just to ensure that removing some checks did the right thing. I’ll remove this and add a check that would have made this eagerly stack overflow

Comment on attachment 310477 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=310477&action=review >>> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:148 >>> + wasmContext->setCachedStackLimit(bitwise_cast<void*>(std::numeric_limits<uintptr_t>::max())); >> >> Why is this correct? What happens if prevWasmContext == wasmContext? Wouldn't this blow away the previous cached stack bounds? >> >> If so, can you add a test? > > This is definitely not correct for the reason you outlined. I added it while testing just to ensure that removing some checks did the right thing. I’ll remove this and add a check that would have made this eagerly stack overflow Oh sorry this is correct because it happens before the storeContext call below it! That said, this is quite tricky and perhaps I should remove it. My thinking when adding this is it’ll make every stack check fail, so if we ever forget to update the bounds, this buys us some safety.

Comment on attachment 310477 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=310477&action=review > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1062 > + m_maxNumJSCallArguments = std::max(m_maxNumJSCallArguments, static_cast<uint32_t>(args.size())); I need to do this for callIndirect as well. I at first convinced myself that this wasn't needed since callIndirect is doing a wasm call, and its stack bound is part of our frame size, however, we might be calling a WebAssemblyWrapperFunction, which is as if we called JS, so we double the stack size. So I need to also do this same math for callIndirect arguments.

Comment on attachment 310477 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=310477&action=review >> JSTests/wasm/function-tests/factorial.js:36 >> +assert.throws(() => fac(10000000000000), RangeError, "Maximum call stack size exceeded."); > > 10,000,000,000,000 way exceeds i32. Can you use 1e9 or something smaller? Sure >> JSTests/wasm/function-tests/stack-overflow.js:39 >> + // FIXME: asssert some stack size > > Bug # This should be removed, I added the code to check for some base stack size. >> JSTests/wasm/function-tests/stack-overflow.js:216 >> + test(42); > > So there are 3 places we want to check for stack overflow: > > * Same instance wasm -> wasm direct call. > * wasm -> wasm indirect call (same or different instance) > * Call of any import (JS or wasm) where the stub is the thing that overflows > > That's tricky because making sure each is tripped is a flaky thing. I'm not sure this test hits all of these? I'd think that you want to test their combination, find the "limit" and then progressively back down, forcing each to fail by trying to cause just that type of call close to the edge of failure... That's kinda hard though! My patch tests these. Note that a stub does not do the bounds check, it's the job of anything calling the stub to take into account the stack space the stub will use. You can see the math to do that with the m_maxNumJSCallArguments field. >>> Source/JavaScriptCore/ChangeLog:11 >>> + runtime option to configure this. >> >> TLS or wasm context (which can sometimes be TLS)? > > Is there an observable difference in performance between these? If not, I'm not sure it's worth the use of our few fast TLS slots. I'll re-run this and get back to y'all. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:411 >> + // 2. Try to speed things up by skipping stack checks. > > Does this work for leaf JS -> wasm? leaf wasm functions never check their stack bounds unless their frame size is larger than ~1024 bytes. So this would be the same here. Note that the entrance to Wasm does a stack check for the frame size needed by the JS entry frame and the first Wasm frame. Because of that, leafs are safe to not have stack checks when called directly from JS since the C++ checks roughly where the stack would be (and it's allowed to be approximate because of our red zone). >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:419 >> + RELEASE_ASSERT(checkSize >= 0); > > Signed overflow is UB, so this check is technically dead. Should be a checked add instead (and the unsigned -> signed coercion could also fail... Not UB, impl-def instead). I'll move to checked.

> >> JSTests/wasm/function-tests/stack-overflow.js:216 > >> + test(42); > > > > So there are 3 places we want to check for stack overflow: > > > > * Same instance wasm -> wasm direct call. > > * wasm -> wasm indirect call (same or different instance) > > * Call of any import (JS or wasm) where the stub is the thing that overflows > > > > That's tricky because making sure each is tripped is a flaky thing. I'm not sure this test hits all of these? I'd think that you want to test their combination, find the "limit" and then progressively back down, forcing each to fail by trying to cause just that type of call close to the edge of failure... That's kinda hard though! > > My patch tests these. Note that a stub does not do the bounds check, it's > the job of anything calling the stub to take into account the stack space > the stub will use. You can see the math to do that with the > m_maxNumJSCallArguments field. That's testing what your current implementation does, assuming no bugs. When possible I'd rather test what any implementation could do, assuming we could have bugs. Granted it's a tricky one here, but I wouldn't just assume that the stubs never need stack space. So I'm OK with what you have now, but if you think it's not too difficult I'd rather have a more thorough test, either here or as a follow-up.

(In reply to JF Bastien from comment #29) > > >> JSTests/wasm/function-tests/stack-overflow.js:216 > > >> + test(42); > > > > > > So there are 3 places we want to check for stack overflow: > > > > > > * Same instance wasm -> wasm direct call. > > > * wasm -> wasm indirect call (same or different instance) > > > * Call of any import (JS or wasm) where the stub is the thing that overflows > > > > > > That's tricky because making sure each is tripped is a flaky thing. I'm not sure this test hits all of these? I'd think that you want to test their combination, find the "limit" and then progressively back down, forcing each to fail by trying to cause just that type of call close to the edge of failure... That's kinda hard though! > > > > My patch tests these. Note that a stub does not do the bounds check, it's > > the job of anything calling the stub to take into account the stack space > > the stub will use. You can see the math to do that with the > > m_maxNumJSCallArguments field. > > That's testing what your current implementation does, assuming no bugs. When > possible I'd rather test what any implementation could do, assuming we could > have bugs. Granted it's a tricky one here, but I wouldn't just assume that > the stubs never need stack space. My patch does not assume this, it assumes the opposite. It assumes it uses numArgs*8 stack space. > > So I'm OK with what you have now, but if you think it's not too difficult > I'd rather have a more thorough test, either here or as a follow-up.

(In reply to Saam Barati from comment #30) > (In reply to JF Bastien from comment #29) > > > >> JSTests/wasm/function-tests/stack-overflow.js:216 > > > >> + test(42); > > > > > > > > So there are 3 places we want to check for stack overflow: > > > > > > > > * Same instance wasm -> wasm direct call. > > > > * wasm -> wasm indirect call (same or different instance) > > > > * Call of any import (JS or wasm) where the stub is the thing that overflows > > > > > > > > That's tricky because making sure each is tripped is a flaky thing. I'm not sure this test hits all of these? I'd think that you want to test their combination, find the "limit" and then progressively back down, forcing each to fail by trying to cause just that type of call close to the edge of failure... That's kinda hard though! > > > > > > My patch tests these. Note that a stub does not do the bounds check, it's > > > the job of anything calling the stub to take into account the stack space > > > the stub will use. You can see the math to do that with the > > > m_maxNumJSCallArguments field. > > > > That's testing what your current implementation does, assuming no bugs. When > > possible I'd rather test what any implementation could do, assuming we could > > have bugs. Granted it's a tricky one here, but I wouldn't just assume that > > the stubs never need stack space. > My patch does not assume this, it assumes the opposite. It assumes it uses > numArgs*8 stack space. This test does? I misunderstood then.

Created attachment 310533 [details] patch for landing

landed in: https://trac.webkit.org/changeset/217060/webkit