Bug 179936 – Fix CLoop::sanitizeStack() bug where it was clearing part of the JS stack in use.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

This issue was uncovered when we enabled --useDollarVM=true on the JSC tests.

Basically, the case of the failing test we observed, op_tail_call_forward_arguments was allocating stack space to stash the arguments (to be forwarded) and new frame info.  The location of this new stash space happens to lie beyond the top of frame of the tail call caller frame.  After stashing the arguments, the code proceeded to load the callee codeBlock.  This triggers an allocation, which in turn, triggers stack sanitization.  The CLoop stack sanitizer was relying on frame->topOfFrame() to tell it where the top of the used stack is.  In this case, that turned out to be inadequate.  As a result, part of the stashed data was zeroed out, and subsequently led to a crash.

This bug does not affect JIT builds for 2 reasons:
1. JIT builds do stack sanitization in the LLInt code itself (different from the CLoop implementation), and the sanitizer there is aware of the true top of stack value (i.e. the stack pointer).
2. JIT builds don't use a parallel stack like the CLoop.  The parallel stack is one condition necessary for reproducing this issue.

The fix is to make the CLoop record the stack pointer in m_stackTop each time before it calls out to native code.  This also brings the CLoops behavior closer to hardware behavior where we always know where the stack pointer is after calling into native C++ code, and make it easier to reason about correctness.

<rdar://problem/35623998>

[Mark Lam]

Created attachment 327446 [details]
proposed patch.

[Saam Barati]

Comment on attachment 327446 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:43
> +            highAddress(): the highest address just beyond the bounds of the stack.

So this value minus one is the stack base?

[WebKit Commit Bot]

Comment on attachment 327446 [details]
proposed patch.

Clearing flags on attachment: 327446

Committed r225140: <https://trac.webkit.org/changeset/225140>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Lam]

(In reply to Saam Barati from comment #2)
> Comment on attachment 327446 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327446&action=review
> 
> > Source/JavaScriptCore/ChangeLog:43
> > +            highAddress(): the highest address just beyond the bounds of the stack.
> 
> So this value minus one is the stack base?

highAddress() points just beyond the stack.
highAddress() - 1 points to the first Register slot on the stack.
If the stackPointer points to highAddress(), then the stack is empty.