Bug 126088 – CStack: callToJavaScript should do stack check for incoming args

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

[Mark Lam]

This change will achieve the following:

1. Change callToJavaScript()'s prototype to:
        EncodedJSValue callToJavaScript(void*, VM*, ProtoCallFrame*);

    We now pass VM* instead of &vm.topCallFrame for the second argument.  This gives us greater utility out of that arg.

2. Change callToJavaScript() to do a stack check to ensure that we have adequate stack space to copy all the args from the protoCallFrame.
    If not, it'll throw a StackOverflowError.

3. Removed JSStack::entryCheck() and calls to it.

    callToJavaScript now takes care of the stack check that ensures adequate stack space for incoming args.
    callToJavaScript does assume that we have adequate stack space for the VMEntrySentinelFrame, but that is ensured by our stack host zone.

[Mark Lam]

Created attachment 219804 [details]
the patch.

[Mark Lam]

Comment on attachment 219804 [details]
the patch.

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

> Source/JavaScriptCore/llint/LLIntSlowPaths.h:126
> +extern "C" void llint_throw_stack_overflow_error(VM*, ProtoCallFrame*);

The style checker was complaining about the use of underscores here.  While the use of underscores is consistent with how LLINT slow paths roll, should I be switching to the webkit style of camel case here?

[Mark Lam]

Landed in r160947 on the jsCStack branch: <http://trac.webkit.org/r160947>.

A review is still needed.

[Michael Saboff]

Comment on attachment 219804 [details]
the patch.

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

r=me modulo that verify that you can safely use vm->topCallFrame in llint_throw_stack_overflow_error

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1394
> +    ExecState* exec = vm->topCallFrame;

Please verify that we can count of vm->topCallFrame being valid or null.  I thought we could only count on its value when we call out to C++.  The JS caller will set topCallFrame before making the call.  I don't think it ever restores it to a prior value.  Seems it could be bad if a JS function A calls B and B calls out to a helper.  B is in now in topCallFrame.  B exits and A now makes a call that happens to be a native function.  I don't think that topCallFrame will get updated again before making the native call.

[Mark Lam]

(In reply to comment #4)
> (From update of attachment 219804 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219804&action=review
> 
> r=me modulo that verify that you can safely use vm->topCallFrame in llint_throw_stack_overflow_error
> 
> > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1394
> > +    ExecState* exec = vm->topCallFrame;
> 
> Please verify that we can count of vm->topCallFrame being valid or null.  I thought we could only count on its value when we call out to C++.  The JS caller will set topCallFrame before making the call.  I don't think it ever restores it to a prior value.  Seems it could be bad if a JS function A calls B and B calls out to a helper.  B is in now in topCallFrame.  B exits and A now makes a call that happens to be a native function.  I don't think that topCallFrame will get updated again before making the native call.

Here are the critical check points:

1. JSStack initializes it to 0 initially.
2. callToJavaScript saves topCallFrame in VMEntrySentinelFrame::ScopeChain upon entry, and restores it before returning.
3. In LLInt slow paths, the NativeCallFrameTracer sets topCallFrame to the current CallFrame before executing native code.
4. JIT operations also use NativeCallFrameTracer to set topCallFrame.

Note that in callToJavaScript, we’ve only just come from C++ code.  Conceptually, we haven’t pushed any JS frames onto the stack yet.  Hence, whatever value topCallFrame had before we called callToJavaScript is the topCallFrame value that we want.  And by design, that’s the one we’re getting (because we haven’t set it to anything else yet). 

We are good to go.

[Michael Saboff]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 219804 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=219804&action=review
> > 
> > r=me modulo that verify that you can safely use vm->topCallFrame in llint_throw_stack_overflow_error
> > 
> > > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1394
> > > +    ExecState* exec = vm->topCallFrame;
> > 
> > Please verify that we can count of vm->topCallFrame being valid or null.  I thought we could only count on its value when we call out to C++.  The JS caller will set topCallFrame before making the call.  I don't think it ever restores it to a prior value.  Seems it could be bad if a JS function A calls B and B calls out to a helper.  B is in now in topCallFrame.  B exits and A now makes a call that happens to be a native function.  I don't think that topCallFrame will get updated again before making the native call.
> 
> Here are the critical check points:
> 
> 1. JSStack initializes it to 0 initially.
> 2. callToJavaScript saves topCallFrame in VMEntrySentinelFrame::ScopeChain upon entry, and restores it before returning.
> 3. In LLInt slow paths, the NativeCallFrameTracer sets topCallFrame to the current CallFrame before executing native code.
> 4. JIT operations also use NativeCallFrameTracer to set topCallFrame.
> 
> Note that in callToJavaScript, we’ve only just come from C++ code.  Conceptually, we haven’t pushed any JS frames onto the stack yet.  Hence, whatever value topCallFrame had before we called callToJavaScript is the topCallFrame value that we want.  And by design, that’s the one we’re getting (because we haven’t set it to anything else yet). 
> 
> We are good to go.

There is also the reentrant case.  When we start off, topCallFrame should be 0.  We save it in callToJavaScript.  The called JS can make a C++ call that will reenter callToJavaScript (e.g. eval).  In that case topCallFrame is fine. 

I think we're okay, because callToJavaScript already relies on topCallFrame being valid.

[Mark Lam]

Thanks for the review.  Review status updated in r160950: <http://trac.webkit.org/r160950>.

[Geoffrey Garen]

Comment on attachment 219804 [details]
the patch.

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

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1392
> +void llint_throw_stack_overflow_error(VM* vm, ProtoCallFrame* protoFrame)

Is there anything that prevents us from passing ExecState* to this function? That would be much better. Then, we could reason about this code without a four-point logic map. Also, that's how other llint_ functions work.

[Mark Lam]

(In reply to comment #8)
> (From update of attachment 219804 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219804&action=review
> 
> > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1392
> > +void llint_throw_stack_overflow_error(VM* vm, ProtoCallFrame* protoFrame)
> 
> Is there anything that prevents us from passing ExecState* to this function? That would be much better. Then, we could reason about this code without a four-point logic map. Also, that's how other llint_ functions work.

I chose to pass a VM* because there’s no guarantee that we have any valid CallFrames on the stack yet, and hence we don’t have a valid ExecState*.  llint_throw_stack_overflow_error() is called from doCallToJavaScript() only if we fail to set up the CallFrame due to inadequate space for the incoming args.  For other llint_ functions, they are guaranteed that there is at least one valid ExecState* that they can use.