Bug 126487 – CStack: The fixed up jsStackLimit in doCallToJavaScript should not exceed the native stack limit.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

When llint_stack_check is called from LowLevelAssembler.asm::functionInitialization it ends up calling VMEntryScope::updateStackLimits() which ASSERTS that the current stack top of frame is within the current stack range.  Were lint_stack_check is called, the codeBlock has been setup for the frame, and therefore the the top of the frame may exceed the stack limit checked in lint_stack_check().  Therefore the ASSERT
    ASSERT(m_vm.interpreter->stack().containsAddress(reinterpret_cast<Register*>(topOfJSStack)));
is not valid.

[Michael Saboff]

Created attachment 220369 [details]
Patch

[Mark Lam]

(In reply to comment #0)
> When llint_stack_check is called from LowLevelAssembler.asm::functionInitialization it ends up calling VMEntryScope::updateStackLimits() which ASSERTS that the current stack top of frame is within the current stack range.  Were lint_stack_check is called, the codeBlock has been setup for the frame, and therefore the the top of the frame may exceed the stack limit checked in lint_stack_check().  Therefore the ASSERT
>     ASSERT(m_vm.interpreter->stack().containsAddress(reinterpret_cast<Register*>(topOfJSStack)));
> is not valid.

I disagree.  In llint_stack_check, we do:

    exec = exec->callerFrame();
    vm.topCallFrame = exec;

before we instantiate "Interpreter::ErrorHandlingMode mode(exec);” which checks the assertion.  Hence, we’re asserting that the caller frame should always be within the bounds of topOfStack as computed by vm.topCallFrame->topOfFrame().  There’s no reason why the caller frame should be on the stack already and exceeding the topOfStack.

There is one possible scenario that I can see violating this assertion, and that is if the caller frame is a VMEntrySentinelFrame.  If so, then I think we’ll have other problems from using it as the ExecState for the subsequent throw operations to follow.

Can you please share the test case that led to this assertion failing?  I would like to investigate this further.  I don’t think removing the assertion is the right thing to do here.

[Michael Saboff]

(In reply to comment #2)
> (In reply to comment #0)
> > When llint_stack_check is called from LowLevelAssembler.asm::functionInitialization it ends up calling VMEntryScope::updateStackLimits() which ASSERTS that the current stack top of frame is within the current stack range.  Were lint_stack_check is called, the codeBlock has been setup for the frame, and therefore the the top of the frame may exceed the stack limit checked in lint_stack_check().  Therefore the ASSERT
> >     ASSERT(m_vm.interpreter->stack().containsAddress(reinterpret_cast<Register*>(topOfJSStack)));
> > is not valid.
> 
> I disagree.  In llint_stack_check, we do:
> 
>     exec = exec->callerFrame();
>     vm.topCallFrame = exec;
> 
> before we instantiate "Interpreter::ErrorHandlingMode mode(exec);” which checks the assertion.  Hence, we’re asserting that the caller frame should always be within the bounds of topOfStack as computed by vm.topCallFrame->topOfFrame().  There’s no reason why the caller frame should be on the stack already and exceeding the topOfStack.
> 
> There is one possible scenario that I can see violating this assertion, and that is if the caller frame is a VMEntrySentinelFrame.  If so, then I think we’ll have other problems from using it as the ExecState for the subsequent throw operations to follow.
> 
> Can you please share the test case that led to this assertion failing?  I would like to investigate this further.  I don’t think removing the assertion is the right thing to do here.

As I said in the description, we exceed the bounds of the stack because the CodeBlock has been set for the callFrame.  In llint_stack_check, we check if the frame pointer, which is in m_vm.topCallFrame, is within the stack bounds.  The ASSERT in question though checks that m_vm.topCallFrame->topOfFrame(), which includes any locals for m_vm.topCallFrame is in bounds.

The failure I was seeing is that the frame pointer is exactly at the edge of the stack bounds, but with the locals it is over the bounds.  This was one of the recursive tests in testapi.

[Mark Lam]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #0)
> > > When llint_stack_check is called from LowLevelAssembler.asm::functionInitialization it ends up calling VMEntryScope::updateStackLimits() which ASSERTS that the current stack top of frame is within the current stack range.  Were lint_stack_check is called, the codeBlock has been setup for the frame, and therefore the the top of the frame may exceed the stack limit checked in lint_stack_check().  Therefore the ASSERT
> > >     ASSERT(m_vm.interpreter->stack().containsAddress(reinterpret_cast<Register*>(topOfJSStack)));
> > > is not valid.
> > 
> > I disagree.  In llint_stack_check, we do:
> > 
> >     exec = exec->callerFrame();
> >     vm.topCallFrame = exec;
> > 
> > before we instantiate "Interpreter::ErrorHandlingMode mode(exec);” which checks the assertion.  Hence, we’re asserting that the caller frame should always be within the bounds of topOfStack as computed by vm.topCallFrame->topOfFrame().  There’s no reason why the caller frame should be on the stack already and exceeding the topOfStack.
> 
> As I said in the description, we exceed the bounds of the stack because the CodeBlock has been set for the callFrame.  In llint_stack_check, we check if the frame pointer, which is in m_vm.topCallFrame, is within the stack bounds.  The ASSERT in question though checks that m_vm.topCallFrame->topOfFrame(), which includes any locals for m_vm.topCallFrame is in bounds.

Indeed.  But as I pointed out, we’re checking the assertion against the caller frame, not the new frame that we’re running functionInitialization() on.  The caller frame should have been stack checked previously and passed the stack check.  Hence, the assertion should not be failing now.  The assertion failure implies that something has gone awry, and that we should investigate it.

> The failure I was seeing is that the frame pointer is exactly at the edge of the stack bounds, but with the locals it is over the bounds.  This was one of the recursive tests in testapi.

I don’t see this assertion failure when I run testapi with a x86_64 debug build on Mavericks.  Are you seeing this as part of the work on the ARM64 port?

[Michael Saboff]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (In reply to comment #0)
> > > > When llint_stack_check is called from LowLevelAssembler.asm::functionInitialization it ends up calling VMEntryScope::updateStackLimits() which ASSERTS that the current stack top of frame is within the current stack range.  Were lint_stack_check is called, the codeBlock has been setup for the frame, and therefore the the top of the frame may exceed the stack limit checked in lint_stack_check().  Therefore the ASSERT
> > > >     ASSERT(m_vm.interpreter->stack().containsAddress(reinterpret_cast<Register*>(topOfJSStack)));
> > > > is not valid.
> > > 
> > > I disagree.  In llint_stack_check, we do:
> > > 
> > >     exec = exec->callerFrame();
> > >     vm.topCallFrame = exec;
> > > 
> > > before we instantiate "Interpreter::ErrorHandlingMode mode(exec);” which checks the assertion.  Hence, we’re asserting that the caller frame should always be within the bounds of topOfStack as computed by vm.topCallFrame->topOfFrame().  There’s no reason why the caller frame should be on the stack already and exceeding the topOfStack.
> > 
> > As I said in the description, we exceed the bounds of the stack because the CodeBlock has been set for the callFrame.  In llint_stack_check, we check if the frame pointer, which is in m_vm.topCallFrame, is within the stack bounds.  The ASSERT in question though checks that m_vm.topCallFrame->topOfFrame(), which includes any locals for m_vm.topCallFrame is in bounds.
> 
> Indeed.  But as I pointed out, we’re checking the assertion against the caller frame, not the new frame that we’re running functionInitialization() on.  The caller frame should have been stack checked previously and passed the stack check.  Hence, the assertion should not be failing now.  The assertion failure implies that something has gone awry, and that we should investigate it.
> 
> > The failure I was seeing is that the frame pointer is exactly at the edge of the stack bounds, but with the locals it is over the bounds.  This was one of the recursive tests in testapi.
> 
> I don’t see this assertion failure when I run testapi with a x86_64 debug build on Mavericks.  Are you seeing this as part of the work on the ARM64 port?

It fails on ARM64.  Try running just testapi LLInt only.  Using the command line 
JSC_useJIT=0 DYLD_FRAMEWORK_PATH=. lldb ./testapi -- testapi.js
I get:
(lldb) r
...
PASS: derivedOnlyDescriptor.value should be 2 and is.
PASS: derivedOnlyDescriptor.configurable should be true and is.
PASS: derivedOnlyDescriptor.enumerable should be false and is.
PASS: undefined instanceof MyObject should be false and is.
ASSERTION FAILED: m_vm.interpreter->stack().containsAddress(reinterpret_cast<Register*>(topOfJSStack))
/Volumes/Data/src/webkit.cstack/Source/JavaScriptCore/runtime/VMEntryScope.cpp(136) : void JSC::VMEntryScope::updateStackLimits()
Process 19432 stopped
* thread #1: tid = 0x3ded04, 0x00000001006b1638 JavaScriptCore`WTFCrash + 68 at Assertions.cpp:341, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbbadbeef)
    frame #0: 0x00000001006b1638 JavaScriptCore`WTFCrash + 68 at Assertions.cpp:341
(lldb) bt 15
* thread #1: tid = 0x3ded04, 0x00000001006b1638 JavaScriptCore`WTFCrash + 68 at Assertions.cpp:341, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbbadbeef)
  * frame #0: 0x00000001006b1638 JavaScriptCore`WTFCrash + 68 at Assertions.cpp:341
    frame #1: 0x000000010048d31c JavaScriptCore`JSC::VMEntryScope::updateStackLimits(this=0x000000016fdc7910) + 616 at VMEntryScope.cpp:136
    frame #2: 0x0000000100472c7c JavaScriptCore`JSC::JSStack::updateStackLimit(this=0x000000015ee078a8) + 80 at JSStack.cpp:208
    frame #3: 0x00000001003924c8 JavaScriptCore`JSC::Interpreter::ErrorHandlingMode::~ErrorHandlingMode(this=0x000000016fcef838) + 144 at Interpreter.cpp:106
    frame #4: 0x000000010039242c JavaScriptCore`JSC::Interpreter::ErrorHandlingMode::~ErrorHandlingMode(this=0x000000016fcef838) + 24 at Interpreter.cpp:102
    frame #5: 0x00000001004cbfac JavaScriptCore`llint_stack_check(exec=0x000000016fcef900, pc=0x000000015ed08a20) + 240 at LLIntSlowPaths.cpp:472
    frame #6: 0x00000001004d6314 JavaScriptCore`llint_function_for_call_prologue + 240
    frame #7: 0x00000001004db244 JavaScriptCore`llint_op_call + 184
    frame #8: 0x00000001004db244 JavaScriptCore`llint_op_call + 184
    frame #9: 0x00000001004db244 JavaScriptCore`llint_op_call + 184
    frame #10: 0x00000001004db244 JavaScriptCore`llint_op_call + 184
    frame #11: 0x00000001004db244 JavaScriptCore`llint_op_call + 184
    frame #12: 0x00000001004db244 JavaScriptCore`llint_op_call + 184
    frame #13: 0x00000001004db244 JavaScriptCore`llint_op_call + 184
    frame #14: 0x00000001004db244 JavaScriptCore`llint_op_call + 184

[Mark Lam]

Comment on attachment 220369 [details]
Patch

The issue is due to a bug in the jsStackLimit fixup code in doCallToJavaScript.  Patch coming soon.

[Mark Lam]

Created attachment 220443 [details]
The jsStackLimit fixup in doCallToJavaScript should not exceed the native stack limit.

[Mark Lam]

The assertion was failing because the jsStackLimit in doCallToJavaScript was erroneously setting the jsStackLimit (lower i.e. allowing more stack space) then the native stack limit.  As a result, the caller frame was allowed to be pushed on to the stack.  The only reason this did not result in a fault yet was because the push frame was taking up space in the host zone of the stack.

Once I fixed the jsStackLimit fix up in doCallToJavaScript to cap it at VM::m_stackLimit, the assertion ceases to fail.  The assertion is valid.

[Michael Saboff]

Comment on attachment 220443 [details]
The jsStackLimit fixup in doCallToJavaScript should not exceed the native stack limit.

View in context: https://bugs.webkit.org/attachment.cgi?id=220443&action=review

r=me with comment fix.

> Source/JavaScriptCore/ChangeLog:11
> +        was missing, and resulted in an assertion failure when running testapi

Duplicate "was"

[Mark Lam]

Thanks for the review.  Landed in r161361 on the jsCStack branch: <http://trac.webkit.org/r161361>.