Bug 100109

Summary:

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

Product:

WebKit

Reporter:

Chris Dumez <cdumez>

Component:

JavaScriptCore

Assignee:

Chris Dumez <cdumez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

fpizlo, ggaren, mark.lam, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

99872

Attachments:

Description	Flags
Patch	none
Patch	none

Chris Dumez

Reported 2012-10-23 04:28:51 PDT

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.

Attachments
Patch (6.12 KB, patch) 2012-10-23 04:40 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (4.58 KB, patch) 2012-10-23 10:00 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Chris Dumez

Comment 1 2012-10-23 04:40:57 PDT

Created attachment 170120 [details] Patch This patch fixes crashing on EFL port.

Chris Dumez

Comment 2 2012-10-23 06:49:10 PDT

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

Mark Lam

Comment 3 2012-10-23 08:20:32 PDT

(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 4 2012-10-23 08:55:29 PDT

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

Comment 5 2012-10-23 09:44:58 PDT

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

Chris Dumez

Comment 6 2012-10-23 10:00:18 PDT

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

Comment 7 2012-10-23 10:03:21 PDT

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 8 2012-10-23 10:58:36 PDT

Comment on attachment 170184 [details] Patch Clearing flags on attachment: 170184 Committed r132244: <http://trac.webkit.org/changeset/132244>

WebKit Review Bot

Comment 9 2012-10-23 10:58:40 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.