Skip to content

Commit 5a03dc3

Browse files
author
Mark Lam
committed
We should use different stack limits for stack checks from JS and host code.
https://bugs.webkit.org/show_bug.cgi?id=159442 <rdar://problem/26889188> Reviewed by Geoffrey Garen. Source/JavaScriptCore: We have 2 stack reservedZoneSizes: 1. Options::softReservedZoneSize() 2. Options::reservedZoneSize() Respectively, there are used to define 2 stack limits based on these reserved zone sizes: 1. VM::m_softStackLimit 2. VM::m_stackLimit Options::reservedZoneSize() is the amount of the stack space that JSC guarantees to the VM and client host code for it's use. Host code that has well known stack usage characteristics (i.e. doesn't call arbitrary code) may do stack checks against the VM::m_stackLimit limit (which is computed using Options::reservedZoneSize()). Options::softReservedZoneSize() is a more conservative amount of reserved stack space. This is used to compute the VM::m_softStackLimit limit. Any code that is difficult to have its stack usage characterized (i.e. may call arbitrary code) may need more stack space for its work. Hence, these should do stack checks against the VM::m_softStackLimit limit. JS code and host code that may call into JS code falls into the category of code that may call arbitrary code. Hence, they should do stack checks against the VM::m_softStackLimit limit. Accordingly, the VM now provides 2 recursion check functions: 1. VM::isSafeToRecurseSoft() will do a stack check against VM::m_softStackLimit. In addition, for C Loop builds, VM::isSafeToRecurseSoft() will also check the CLoopStack against VM::m_cloopStackLimit. 2. VM::isSafeToRecurse() will do a stack check against VM::m_stackLimit. Also added a promise-infinite-recursion-should-not-crash.js test. * bytecompiler/BytecodeGenerator.h: (JSC::BytecodeGenerator::emitNodeInTailPosition): (JSC::BytecodeGenerator::emitNodeInConditionContext): * interpreter/CLoopStack.cpp: (JSC::CLoopStack::grow): * interpreter/CLoopStack.h: (JSC::CLoopStack::size): * interpreter/CLoopStackInlines.h: (JSC::CLoopStack::ensureCapacityFor): (JSC::CLoopStack::isSafeToRecurse): (JSC::CLoopStack::topOfFrameFor): * interpreter/CachedCall.h: (JSC::CachedCall::CachedCall): * interpreter/Interpreter.cpp: (JSC::Interpreter::execute): (JSC::Interpreter::executeCall): (JSC::Interpreter::executeConstruct): * llint/LLIntSlowPaths.cpp: (JSC::LLInt::LLINT_SLOW_PATH_DECL): * parser/Parser.cpp: * runtime/Options.h: * runtime/ProxyObject.cpp: (JSC::performProxyGet): (JSC::ProxyObject::performInternalMethodGetOwnProperty): (JSC::ProxyObject::performHasProperty): (JSC::ProxyObject::getOwnPropertySlotCommon): (JSC::ProxyObject::performPut): (JSC::performProxyCall): (JSC::performProxyConstruct): (JSC::ProxyObject::performDelete): (JSC::ProxyObject::performPreventExtensions): (JSC::ProxyObject::performIsExtensible): (JSC::ProxyObject::performDefineOwnProperty): (JSC::ProxyObject::performGetOwnPropertyNames): (JSC::ProxyObject::performSetPrototype): (JSC::ProxyObject::performGetPrototype): * runtime/RegExp.cpp: (JSC::RegExp::finishCreation): (JSC::RegExp::compile): (JSC::RegExp::compileMatchOnly): * runtime/StringRecursionChecker.h: (JSC::StringRecursionChecker::performCheck): * runtime/VM.cpp: (JSC::VM::setStackPointerAtVMEntry): (JSC::VM::updateSoftReservedZoneSize): (JSC::preCommitStackMemory): (JSC::VM::updateStackLimits): (JSC::VM::updateStackLimit): Deleted. * runtime/VM.h: (JSC::VM::stackLimit): (JSC::VM::softStackLimit): (JSC::VM::addressOfSoftStackLimit): (JSC::VM::setCLoopStackLimit): (JSC::VM::isSafeToRecurse): (JSC::VM::lastStackTop): (JSC::VM::setException): * runtime/VMInlines.h: (JSC::VM::ensureStackCapacityFor): (JSC::VM::isSafeToRecurseSoft): (JSC::VM::shouldTriggerTermination): * tests/stress/promise-infinite-recursion-should-not-crash.js: Added. (testPromise): (promiseFunc): * yarr/YarrPattern.cpp: Tools: In http://trac.webkit.org/r203067, we limited the amount of stack that tests will run with to keep stack overflow tests sane. Turns out, we also need to teach the LayoutTestRelay to pass env vars over to the iOS simulator. This is needed in order to keep the js/regress-139548.html test happy with this patch. Also fixed up run_webkit_tests.py to explicitly pass an int size value for the JSC_maxPerThreadStackUsage option. Otherwise, it will pass a float value. * LayoutTestRelay/LayoutTestRelay/LTRelayController.m: (-[LTRelayController _environmentVariables]): * Scripts/webkitpy/layout_tests/run_webkit_tests.py: (main): LayoutTests: * js/regress-141098-expected.txt: * js/script-tests/regress-141098.js: (testEval): (probeAndRecurse): - Gave all the test constants names. - Tweaked the constants to allow the test to run in the least amount of time, and also to behave consistently across all test configurations. - Re-enable eager tests now that the test should finish quickly. Canonical link: https://commits.webkit.org/177842@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@203142 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent e91b189 commit 5a03dc3

24 files changed

Lines changed: 309 additions & 63 deletions

File tree

‎LayoutTests/ChangeLog‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,20 @@
1+
2016-07-12 Mark Lam <mark.lam@apple.com>
2+
3+
We should use different stack limits for stack checks from JS and host code.
4+
https://bugs.webkit.org/show_bug.cgi?id=159442
5+
<rdar://problem/26889188>
6+
7+
Reviewed by Geoffrey Garen.
8+
9+
* js/regress-141098-expected.txt:
10+
* js/script-tests/regress-141098.js:
11+
(testEval):
12+
(probeAndRecurse):
13+
- Gave all the test constants names.
14+
- Tweaked the constants to allow the test to run in the least amount of time, and
15+
also to behave consistently across all test configurations.
16+
- Re-enable eager tests now that the test should finish quickly.
17+
118
2016-07-12 Dean Jackson <dino@apple.com>
219

320
REGRESSION (202694): Audio and Video playback controls: Cannot find a position slider to adjust playback position using VO.

‎LayoutTests/js/regress-141098-expected.txt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE
44

55

66
PASS Initial setup
7+
Starting 1st probeAndRecurse
78
PASS Exception: RangeError: Maximum call stack size exceeded.
9+
Starting 2nd probeAndRecurse
810
PASS Exception: RangeError: Maximum call stack size exceeded.
911
PASS successfullyParsed is true
1012

‎LayoutTests/js/script-tests/regress-141098.js‎

Lines changed: 34 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,36 @@
1-
//@ noEagerNoNoLLIntTestsRunLayoutTest
1+
//@ noNoLLIntRunLayoutTest
22

33
description("Regression test for https://webkit.org/b/141098. Make sure eval() properly handles running out of stack space. This test should run without crashing.");
44

55
// The tiering up to test higher levels of optimization will only test the DFG
66
// if run in run-jsc-stress-tests with the eager settings.
77

8+
// countStart, countIncrement, and numberOfFramesToBackoffFromStackOverflowPoint were
9+
// empirically determined to be the values that will cause StackOverflowErrors to be
10+
// thrown where the tests expects them. If stack frame layouts change sufficiently (e.g.
11+
// due to JIT changes) such that this test starts failing (due to text output
12+
// differences), then these values will need to be rebased.
13+
// Under no circumstance should this test ever crash.
14+
let countStart = 2;
15+
let countIncrement = 8;
16+
let numberOfFramesToBackoffFromStackOverflowPoint = 50;
17+
18+
// backoffEverything is chosen to be -1 because a negative number will never be
19+
// decremented to 0, and hence, will cause probeAndRecurse() to return out of every
20+
// frame instead of retrying the eval test.
21+
let backoffEverything = -1;
22+
823
var lastEvalString = "";
924

1025
function testEval(maxIterations)
1126
{
1227
var result;
13-
var count = 1;
28+
var count = countStart;
1429

1530
if (!maxIterations)
1631
var result = eval(lastEvalString);
1732
else {
18-
for (var iter = 0; iter < maxIterations; count *= 4, iter++) {
33+
for (var iter = 0; iter < maxIterations; count *= countIncrement, iter++) {
1934
var evalString = "\"dummy\".valueOf(";
2035

2136
for (var i = 0; i < count; i++) {
@@ -35,26 +50,29 @@ function testEval(maxIterations)
3550
return result;
3651
}
3752

38-
function probeAndRecurse(depth, reuseEvalString)
53+
function probeAndRecurse(reuseEvalString)
3954
{
40-
var result;
55+
var framesToBackOffFromStackOverflowPoint;
4156

4257
// Probe stack depth
4358
try {
44-
result = probeAndRecurse(depth+1, reuseEvalString);
59+
remainingFramesToBackOff = probeAndRecurse(reuseEvalString);
4560

46-
if (!result) {
61+
if (!remainingFramesToBackOff) {
62+
// We've backed off a number of frames. Now retry the eval test to see if we
63+
// still overflow.
4764
try {
4865
testEval(1);
4966
} catch (e) {
50-
return -49;
67+
// Yikes. We still overflow. Back off some more.
68+
return numberOfFramesToBackoffFromStackOverflowPoint;
5169
}
5270
} else
53-
return result + 1
71+
return remainingFramesToBackOff - 1
5472
} catch (e) {
5573
// We exceeded stack space, now return up the stack until we can execute a simple eval.
5674
// Then run an eval test to exceed stack.
57-
return -49;
75+
return numberOfFramesToBackoffFromStackOverflowPoint;
5876
}
5977

6078
try {
@@ -63,20 +81,22 @@ function probeAndRecurse(depth, reuseEvalString)
6381
testPassed("Exception: " + e);
6482
}
6583

66-
return 1;
84+
return backoffEverything;
6785
}
6886

69-
// Because this test intentionlly exhausts the stack, we call testPassed() to ensure
87+
// Because this test intentionally exhausts the stack, we call testPassed() to ensure
7088
// everything we need in that path has been compiled and is available. Otherwise we
7189
// might properly handle an out of stack, but run out of stack calling testPassed().
7290
testPassed("Initial setup");
7391

74-
var depth = probeAndRecurse(0, false);
92+
debug("Starting 1st probeAndRecurse");
93+
probeAndRecurse(false);
7594

7695
// Tier up the eval'ed code.
7796
// When run with run-jsc-stress-tests and it's agressive options, this low of a count will
7897
// allow us to get up to the DFG.
7998
for (var i = 0; i < 200; i++)
8099
testEval(0);
81100

82-
probeAndRecurse(0, true);
101+
debug("Starting 2nd probeAndRecurse");
102+
probeAndRecurse(true);

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,111 @@
1+
2016-07-12 Mark Lam <mark.lam@apple.com>
2+
3+
We should use different stack limits for stack checks from JS and host code.
4+
https://bugs.webkit.org/show_bug.cgi?id=159442
5+
<rdar://problem/26889188>
6+
7+
Reviewed by Geoffrey Garen.
8+
9+
We have 2 stack reservedZoneSizes:
10+
1. Options::softReservedZoneSize()
11+
2. Options::reservedZoneSize()
12+
13+
Respectively, there are used to define 2 stack limits based on these reserved
14+
zone sizes:
15+
1. VM::m_softStackLimit
16+
2. VM::m_stackLimit
17+
18+
Options::reservedZoneSize() is the amount of the stack space that JSC guarantees
19+
to the VM and client host code for it's use. Host code that has well known
20+
stack usage characteristics (i.e. doesn't call arbitrary code) may do stack
21+
checks against the VM::m_stackLimit limit (which is computed using
22+
Options::reservedZoneSize()).
23+
24+
Options::softReservedZoneSize() is a more conservative amount of reserved stack
25+
space. This is used to compute the VM::m_softStackLimit limit. Any code that
26+
is difficult to have its stack usage characterized (i.e. may call arbitrary code)
27+
may need more stack space for its work. Hence, these should do stack checks
28+
against the VM::m_softStackLimit limit.
29+
30+
JS code and host code that may call into JS code falls into the category of code
31+
that may call arbitrary code. Hence, they should do stack checks against the
32+
VM::m_softStackLimit limit.
33+
34+
Accordingly, the VM now provides 2 recursion check functions:
35+
36+
1. VM::isSafeToRecurseSoft() will do a stack check against VM::m_softStackLimit.
37+
In addition, for C Loop builds, VM::isSafeToRecurseSoft() will also
38+
check the CLoopStack against VM::m_cloopStackLimit.
39+
40+
2. VM::isSafeToRecurse() will do a stack check against VM::m_stackLimit.
41+
42+
Also added a promise-infinite-recursion-should-not-crash.js test.
43+
44+
* bytecompiler/BytecodeGenerator.h:
45+
(JSC::BytecodeGenerator::emitNodeInTailPosition):
46+
(JSC::BytecodeGenerator::emitNodeInConditionContext):
47+
* interpreter/CLoopStack.cpp:
48+
(JSC::CLoopStack::grow):
49+
* interpreter/CLoopStack.h:
50+
(JSC::CLoopStack::size):
51+
* interpreter/CLoopStackInlines.h:
52+
(JSC::CLoopStack::ensureCapacityFor):
53+
(JSC::CLoopStack::isSafeToRecurse):
54+
(JSC::CLoopStack::topOfFrameFor):
55+
* interpreter/CachedCall.h:
56+
(JSC::CachedCall::CachedCall):
57+
* interpreter/Interpreter.cpp:
58+
(JSC::Interpreter::execute):
59+
(JSC::Interpreter::executeCall):
60+
(JSC::Interpreter::executeConstruct):
61+
* llint/LLIntSlowPaths.cpp:
62+
(JSC::LLInt::LLINT_SLOW_PATH_DECL):
63+
* parser/Parser.cpp:
64+
* runtime/Options.h:
65+
* runtime/ProxyObject.cpp:
66+
(JSC::performProxyGet):
67+
(JSC::ProxyObject::performInternalMethodGetOwnProperty):
68+
(JSC::ProxyObject::performHasProperty):
69+
(JSC::ProxyObject::getOwnPropertySlotCommon):
70+
(JSC::ProxyObject::performPut):
71+
(JSC::performProxyCall):
72+
(JSC::performProxyConstruct):
73+
(JSC::ProxyObject::performDelete):
74+
(JSC::ProxyObject::performPreventExtensions):
75+
(JSC::ProxyObject::performIsExtensible):
76+
(JSC::ProxyObject::performDefineOwnProperty):
77+
(JSC::ProxyObject::performGetOwnPropertyNames):
78+
(JSC::ProxyObject::performSetPrototype):
79+
(JSC::ProxyObject::performGetPrototype):
80+
* runtime/RegExp.cpp:
81+
(JSC::RegExp::finishCreation):
82+
(JSC::RegExp::compile):
83+
(JSC::RegExp::compileMatchOnly):
84+
* runtime/StringRecursionChecker.h:
85+
(JSC::StringRecursionChecker::performCheck):
86+
* runtime/VM.cpp:
87+
(JSC::VM::setStackPointerAtVMEntry):
88+
(JSC::VM::updateSoftReservedZoneSize):
89+
(JSC::preCommitStackMemory):
90+
(JSC::VM::updateStackLimits):
91+
(JSC::VM::updateStackLimit): Deleted.
92+
* runtime/VM.h:
93+
(JSC::VM::stackLimit):
94+
(JSC::VM::softStackLimit):
95+
(JSC::VM::addressOfSoftStackLimit):
96+
(JSC::VM::setCLoopStackLimit):
97+
(JSC::VM::isSafeToRecurse):
98+
(JSC::VM::lastStackTop):
99+
(JSC::VM::setException):
100+
* runtime/VMInlines.h:
101+
(JSC::VM::ensureStackCapacityFor):
102+
(JSC::VM::isSafeToRecurseSoft):
103+
(JSC::VM::shouldTriggerTermination):
104+
* tests/stress/promise-infinite-recursion-should-not-crash.js: Added.
105+
(testPromise):
106+
(promiseFunc):
107+
* yarr/YarrPattern.cpp:
108+
1109
2016-07-12 Per Arne Vollan <pvollan@apple.com>
2110

3111
[Win] Fix for build error when trying to version stamp dll.

‎Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -384,7 +384,7 @@ namespace JSC {
384384
{
385385
// Node::emitCode assumes that dst, if provided, is either a local or a referenced temporary.
386386
ASSERT(!dst || dst == ignoredResult() || !dst->isTemporary() || dst->refCount());
387-
if (!m_vm->isSafeToRecurse()) {
387+
if (UNLIKELY(!m_vm->isSafeToRecurse())) {
388388
emitThrowExpressionTooDeepException();
389389
return;
390390
}
@@ -411,7 +411,7 @@ namespace JSC {
411411
{
412412
// Node::emitCode assumes that dst, if provided, is either a local or a referenced temporary.
413413
ASSERT(!dst || dst == ignoredResult() || !dst->isTemporary() || dst->refCount());
414-
if (!m_vm->isSafeToRecurse())
414+
if (UNLIKELY(!m_vm->isSafeToRecurse()))
415415
return emitThrowExpressionTooDeepException();
416416
return n->emitBytecode(*this, dst);
417417
}
@@ -428,7 +428,7 @@ namespace JSC {
428428

429429
void emitNodeInConditionContext(ExpressionNode* n, Label* trueTarget, Label* falseTarget, FallThroughMode fallThroughMode)
430430
{
431-
if (!m_vm->isSafeToRecurse()) {
431+
if (UNLIKELY(!m_vm->isSafeToRecurse())) {
432432
emitThrowExpressionTooDeepException();
433433
return;
434434
}

‎Source/JavaScriptCore/interpreter/CLoopStack.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ bool CLoopStack::grow(Register* newTopOfStack)
8888
}
8989

9090
// Compute the chunk size of additional memory to commit, and see if we
91-
// have it is still within our budget. If not, we'll fail to grow and
91+
// have it still within our budget. If not, we'll fail to grow and
9292
// return false.
9393
ptrdiff_t delta = reinterpret_cast<char*>(m_commitTop) - reinterpret_cast<char*>(newTopOfStackWithReservedZone);
9494
delta = WTF::roundUpToMultipleOf(commitSize(), delta);

‎Source/JavaScriptCore/interpreter/CLoopStack.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ namespace JSC {
6868
size_t size() const { return highAddress() - lowAddress(); }
6969

7070
void setSoftReservedZoneSize(size_t);
71+
bool isSafeToRecurse() const;
7172

7273
inline Register* topOfStack();
7374

‎Source/JavaScriptCore/interpreter/CLoopStackInlines.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,12 @@ inline bool CLoopStack::ensureCapacityFor(Register* newTopOfStack)
4242
return grow(newTopOfStack);
4343
}
4444

45+
bool CLoopStack::isSafeToRecurse() const
46+
{
47+
void* reservationLimit = reinterpret_cast<int8_t*>(reservationTop() + m_reservedZoneSizeInRegisters);
48+
return !m_topCallFrame || (m_topCallFrame->topOfFrame() > reservationLimit);
49+
}
50+
4551
inline Register* CLoopStack::topOfFrameFor(CallFrame* frame)
4652
{
4753
if (UNLIKELY(!frame))

‎Source/JavaScriptCore/interpreter/CachedCall.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2009, 2013 Apple Inc. All rights reserved.
2+
* Copyright (C) 2009, 2013, 2016 Apple Inc. All rights reserved.
33
*
44
* Redistribution and use in source and binary forms, with or without
55
* modification, are permitted provided that the following conditions
@@ -33,6 +33,7 @@
3333
#include "Interpreter.h"
3434
#include "ProtoCallFrame.h"
3535
#include "VMEntryScope.h"
36+
#include "VMInlines.h"
3637

3738
namespace JSC {
3839
class CachedCall {
@@ -44,7 +45,7 @@ namespace JSC {
4445
, m_entryScope(callFrame->vm(), function->scope()->globalObject())
4546
{
4647
ASSERT(!function->isHostFunctionNonInline());
47-
if (callFrame->vm().isSafeToRecurse()) {
48+
if (UNLIKELY(callFrame->vm().isSafeToRecurseSoft())) {
4849
m_arguments.resize(argumentCount);
4950
m_closure = m_interpreter->prepareForRepeatCall(function->jsExecutable(), callFrame, &m_protoCallFrame, function, argumentCount + 1, function->scope(), m_arguments.data());
5051
} else

‎Source/JavaScriptCore/interpreter/Interpreter.cpp‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -828,7 +828,7 @@ JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, J
828828
if (vm.isCollectorBusy())
829829
return jsNull();
830830

831-
if (!vm.isSafeToRecurse())
831+
if (UNLIKELY(!vm.isSafeToRecurseSoft()))
832832
return checkedReturn(throwStackOverflowError(callFrame));
833833

834834
// First check if the "program" is actually just a JSON object. If so,
@@ -988,7 +988,7 @@ JSValue Interpreter::executeCall(CallFrame* callFrame, JSObject* function, CallT
988988
}
989989

990990
VMEntryScope entryScope(vm, globalObject);
991-
if (!vm.isSafeToRecurse())
991+
if (UNLIKELY(!vm.isSafeToRecurseSoft()))
992992
return checkedReturn(throwStackOverflowError(callFrame));
993993

994994
if (isJSCall) {
@@ -1050,7 +1050,7 @@ JSObject* Interpreter::executeConstruct(CallFrame* callFrame, JSObject* construc
10501050
}
10511051

10521052
VMEntryScope entryScope(vm, globalObject);
1053-
if (!vm.isSafeToRecurse())
1053+
if (UNLIKELY(!vm.isSafeToRecurseSoft()))
10541054
return checkedReturn(throwStackOverflowError(callFrame));
10551055

10561056
if (isJSConstruct) {
@@ -1147,7 +1147,7 @@ JSValue Interpreter::execute(EvalExecutable* eval, CallFrame* callFrame, JSValue
11471147
return jsNull();
11481148

11491149
VMEntryScope entryScope(vm, scope->globalObject());
1150-
if (!vm.isSafeToRecurse())
1150+
if (UNLIKELY(!vm.isSafeToRecurseSoft()))
11511151
return checkedReturn(throwStackOverflowError(callFrame));
11521152

11531153
unsigned numVariables = eval->numVariables();
@@ -1248,7 +1248,7 @@ JSValue Interpreter::execute(ModuleProgramExecutable* executable, CallFrame* cal
12481248
return jsNull();
12491249

12501250
VMEntryScope entryScope(vm, scope->globalObject());
1251-
if (!vm.isSafeToRecurse())
1251+
if (UNLIKELY(!vm.isSafeToRecurseSoft()))
12521252
return checkedReturn(throwStackOverflowError(callFrame));
12531253

12541254
JSObject* compileError = executable->prepareForExecution(callFrame, nullptr, scope, CodeForCall);

0 commit comments

Comments
 (0)