Bug 100109 – Regression(r132143): Assertion hit in JSC::Interpreter::StackPolicy::StackPolicy(JSC::Interpreter&, const WTF::StackBounds&)

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Chris Dumez]

On EFL 64bit debug build bot, we hit the following assertion after r132143:
ASSERTION FAILED: (requiredCapacity >= 0) && (requiredCapacity < size)
/home/chris/Devel/WebKit/Source/JavaScriptCore/interpreter/Interpreter.cpp(194) : JSC::Interpreter::StackPolicy::StackPolicy(JSC::Interpreter&, const WTF::StackBounds&)
1   0x2b6781fbd5a6 JSC::Interpreter::StackPolicy::StackPolicy(JSC::Interpreter&, WTF::StackBounds const&)
2   0x2b6781fc095c JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*)
3   0x2b6782080af4 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*)
4   0x2b677e445e65 WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*)
5   0x2b677e462bbc WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld*)
6   0x2b677e462cbe WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&)
7   0x2b677d70490c WebCore::ScriptController::executeScript(WebCore::ScriptSourceCode const&)
8   0x2b677d704862 WebCore::ScriptController::executeScript(WTF::String const&, bool)
9   0x2b677aa55aa9 WebKit::WebPage::runJavaScriptInMainFrame(WTF::String const&, unsigned long)
10  0x2b677aae383e void CoreIPC::callMemberFunction<WebKit::WebPage, void (WebKit::WebPage::*)(WTF::String const&, unsigned long), WTF::String, unsigned long>(CoreIPC::Arguments2<WTF::String, unsigned long> const&, WebKit::WebPage*, void (WebKit::WebPage::*)(WTF::String const&, unsigned long))
11  0x2b677aae159a void CoreIPC::handleMessage<Messages::WebPage::RunJavaScriptInMainFrame, WebKit::WebPage, void (WebKit::WebPage::*)(WTF::String const&, unsigned long)>(CoreIPC::MessageDecoder&, WebKit::WebPage*, void (WebKit::WebPage::*)(WTF::String const&, unsigned long))
12  0x2b677aadf44e WebKit::WebPage::didReceiveWebPageMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
13  0x2b677aa58d2e WebKit::WebPage::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
14  0x2b677a988b88 WebKit::WebProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
15  0x2b677a98459f WebKit::WebConnectionToUIProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
16  0x2b677a84bb4c CoreIPC::Connection::dispatchMessage(CoreIPC::MessageID, CoreIPC::MessageDecoder&)
17  0x2b677a84bc85 CoreIPC::Connection::dispatchMessage(CoreIPC::Connection::Message<CoreIPC::MessageDecoder>&)
18  0x2b677a84be21 CoreIPC::Connection::dispatchOneMessage()
19  0x2b677a855f76 WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>::operator()(CoreIPC::Connection*)
20  0x2b677a855d7c WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>, void (CoreIPC::Connection*)>::operator()()
21  0x2b677aa3775c WTF::Function<void ()>::operator()() const
22  0x2b677deee132 WebCore::RunLoop::performWork()
23  0x2b677e8a88a7 WebCore::RunLoop::wakeUpEvent(void*, void*, unsigned int)
24  0x2b6780d28901
25  0x2b6780d27851
26  0x2b6780d27d97 ecore_main_loop_begin
27  0x2b677e8a8871 WebCore::RunLoop::run()
28  0x2b677aab4d78 WebProcessMainEfl
29  0x4007c4 main
30  0x2b677ae7e76d __libc_start_main
31  0x4006e9

When we hit this assertion, the size is negative which seems to indicate a possible integer overflow.

[Chris Dumez]

Created attachment 170120 [details]
Patch

This patch fixes crashing on EFL port.

[Chris Dumez]

On my machine I got before the crash:
stack.size(): 92967688560640
Max INT:          2147483647

[Mark Lam]

(In reply to comment #2)
> On my machine I got before the crash:
> stack.size(): 92967688560640
> Max INT:          2147483647

Thanks for catching this issue.  I didn't think it was possible (because it seemed highly impractical) that someone would want to have a stack that is greater than 2G.  Your case proves otherwise, although I would be suspicious of that size value.  92967688560640 makes for 84 terabytes of memory.  That's more memory than most machines have just to be used for a single stack.

While it is programmatically legal for you to set the stack limit/size this high, I suspect that in practice, what this actually means is that on your platform, the stack check will be defeated, and you are effectively not doing any checks at all.  One side effect of this is that some of the layout tests will start to fail in a flaky manner.  The reason is because, by defeating the stack check with an artificially high stack size, you allow the code to run with a stack check.  As a result, the test will fail and crash wherever you physically run out of stack space, which is long before you reached the specified limit, and which may vary depending on system load and available memory at the time of the run.

And again, I point you to StackBounds.  StackBounds::initialize() is platform dependent.  I suggest that you add something for PLATFORM(EFL) there that bounds the stack to a more reasonable limit so that the stack recursion checks can be effective.

Regardless, you are right that I should have used size_t instead of int for the StackPolicy calculations.  I will comment on your patch shortly.  Thanks again for catching this issue.

[Mark Lam]

Comment on attachment 170120 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:14
> +        happy.

See comment below.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:123
> +    const size_t DefaultErrorModeRequiredStack = 32 * 1024;

I prefer that you leave these capitalized as in the original.  I understand that the style checker is not happy about it, but I consider this a bug in the style checker rather than in the code, and I made a comment about it in my original patch here: https://bugs.webkit.org/show_bug.cgi?id=99872#c3

> Source/JavaScriptCore/interpreter/Interpreter.cpp:128
> +    //    but require that we have at least DefaultRequiredStack capacity

Ditto.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:133
> +    //    |         ... | <-- DefaultRequiredStack --> | ...

Ditto.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:146
> +    //    DefaultErrorModeRequiredStack capacity.

Ditto, and similarly below.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:180
> +    size_t useableStack = (requiredCapacity <= size) ? size - requiredCapacity : DefaultMinimumUseableStack;

Nit: (not in the coding style, just my personal pref but ...) can you break this line into 2 lines (as in example above) so as to keep the lengths < ~80 per line.  That was one of my goals when I wrote this function.  Would like to keep it consistent if that's ok with you.  Thanks.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:191
> +    ASSERT(useableStack >= DefaultMinimumUseableStack && useableStack <= size);

This assertion is not true.  If the stack size is less than DEFAULT_MINIMUM_USEABLE_STACK, then (useableStack >= DEFAULT_MINIMUM_USEABLE_STACK) will never be true.  Please fix the assertion or delete it.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:195
>      // interpreter stack checks:

Please delete this line.  It was a half edited comment from my patch.  Thanks.

LGTM after you address the above issues.  Thanks.

[Chris Dumez]

Ok, thanks. I'll fix and reupload now.

[Chris Dumez]

Created attachment 170184 [details]
Patch

Take Mark Lam's feedback into consideration.

BTW, the extremely high stack size value is returned by pthread_attr_getstack(&sattr, &stackBase, &stackSize) (in #elif OS(UNIX) implementation of StackBounds::initialize()). I'll look further into it but this is anyway a separate issue.

[WebKit Review Bot]

Attachment 170184 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/interpreter/Interpreter.cpp:121:  DEFAULT_REQUIRED_STACK is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/interpreter/Interpreter.cpp:122:  DEFAULT_MINIMUM_USEABLE_STACK is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/interpreter/Interpreter.cpp:123:  DEFAULT_ERROR_MODE_REQUIRED_STACK is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 170184 [details]
Patch

Clearing flags on attachment: 170184

Committed r132244: <http://trac.webkit.org/changeset/132244>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.