Created attachment 295490 [details] POC to trigger out of bounds read (jsc) Affected SVN revision: 208970 To reproduce the problem: ./jsc jsc_operator_string_segfault.js ASAN Output: ==27690==ERROR: AddressSanitizer: SEGV on unknown address 0x0000977537dd (pc 0x7ff9b41478f7 bp 0x7fff076ec010 sp 0x7fff076eb220 T0) ==27690==The signal is caused by a READ memory access. #0 0x7ff9b41478f6 in WTFCrash XYZ/WebKit/Source/WTF/wtf/Assertions.cpp:322:5 #1 0x7ff9b2f9e230 in JSC::operatorString(bool, unsigned int) XYZ/WebKit/Source/JavaScriptCore/parser/Parser.cpp:4625:5 #2 0x7ff9b2f9e230 in JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseUnaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) XYZ/WebKit/Source/JavaScriptCore/parser/Parser.cpp:4675 #3 0x7ff9b2f9e230 in JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseBinaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) XYZ/WebKit/Source/JavaScriptCore/parser/Parser.cpp:3658 #4 0x7ff9b2f9e230 in JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseConditionalExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) XYZ/WebKit/Source/JavaScriptCore/parser/Parser.cpp:3614 #5 0x7ff9b2f9e230 in JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::Parser<JSC::Lexer<unsigned char> >::ExpressionErrorClassifier&) XYZ/WebKit/Source/JavaScriptCore/parser/Parser.cpp:3454 #6 0x7ff9b2ea0cae in JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) XYZ/WebKit/Source/JavaScriptCore/parser/Parser.cpp:3423:12 #7 0x7ff9b2ea0cae in JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) XYZ/WebKit/Source/JavaScriptCore/parser/Parser.cpp:3385 #8 0x7ff9b301081b in JSC::ASTBuilder::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseExpressionStatement<JSC::ASTBuilder>(JSC::ASTBuilder&) XYZ/WebKit/Source/JavaScriptCore/parser/Parser.cpp:2883:33 #9 0x7ff9b2fe070a in JSC::ASTBuilder::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseStatement<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::Identifier const*&, unsigned int*) XYZ/WebKit/Source/JavaScriptCore/parser/Parser.cpp:1806:39 #10 0x7ff9b2fc3ed6 in JSC::ASTBuilder::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseStatementListItem<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::Identifier const*&, unsigned int*) XYZ/WebKit/Source/JavaScriptCore/parser/Parser.cpp:669:18 #11 0x7ff9b2e083d2 in JSC::ASTBuilder::SourceElements JSC::Parser<JSC::Lexer<unsigned char> >::parseSourceElements<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::SourceElementsMode) XYZ/WebKit/Source/JavaScriptCore/parser/Parser.cpp:410:38 #12 0x7ff9b2df5391 in JSC::Parser<JSC::Lexer<unsigned char> >::parseInner(JSC::Identifier const&, JSC::SourceParseMode) XYZ/WebKit/Source/JavaScriptCore/parser/Parser.cpp:295:30 #13 0x7ff9afddeb20 in std::unique_ptr<JSC::ProgramNode, std::default_delete<JSC::ProgramNode> > JSC::Parser<JSC::Lexer<unsigned char> >::parse<JSC::ProgramNode>(JSC::ParserError&, JSC::Identifier const&, JSC::SourceParseMode) XYZ/WebKit/Source/JavaScriptCore/parser/Parser.h:1796:25 #14 0x7ff9afdde2fc in std::unique_ptr<JSC::ProgramNode, std::default_delete<JSC::ProgramNode> > JSC::parse<JSC::ProgramNode>(JSC::VM*, JSC::SourceCode const&, JSC::Identifier const&, JSC::JSParserBuiltinMode, JSC::JSParserStrictMode, JSC::JSParserScriptMode, JSC::SourceParseMode, JSC::SuperBinding, JSC::ParserError&, JSC::JSTextPosition*, JSC::ConstructorKind, JSC::DerivedContextType, JSC::EvalContextType, JSC::DebuggerParseData*) XYZ/WebKit/Source/JavaScriptCore/parser/Parser.h:1885:53 #15 0x7ff9b32b1937 in JSC::UnlinkedProgramCodeBlock* JSC::generateUnlinkedCodeBlock<JSC::UnlinkedProgramCodeBlock, JSC::ProgramExecutable>(JSC::VM&, JSC::ProgramExecutable*, JSC::SourceCode const&, JSC::JSParserStrictMode, JSC::JSParserScriptMode, JSC::DebuggerMode, JSC::ParserError&, JSC::EvalContextType, JSC::VariableEnvironment const*) XYZ/WebKit/Source/JavaScriptCore/runtime/CodeCache.h:235:42 #16 0x7ff9b32a76d3 in JSC::UnlinkedProgramCodeBlock* JSC::CodeCache::getUnlinkedGlobalCodeBlock<JSC::UnlinkedProgramCodeBlock, JSC::ProgramExecutable>(JSC::VM&, JSC::ProgramExecutable*, JSC::SourceCode const&, JSC::JSParserStrictMode, JSC::JSParserScriptMode, JSC::DebuggerMode, JSC::ParserError&, JSC::EvalContextType) XYZ/WebKit/Source/JavaScriptCore/runtime/CodeCache.cpp:75:48 #17 0x7ff9b32a381f in JSC::CodeCache::getUnlinkedProgramCodeBlock(JSC::VM&, JSC::ProgramExecutable*, JSC::SourceCode const&, JSC::JSParserStrictMode, JSC::DebuggerMode, JSC::ParserError&) XYZ/WebKit/Source/JavaScriptCore/runtime/CodeCache.cpp:85:12 #18 0x7ff9b3699f25 in JSC::JSGlobalObject::createProgramCodeBlock(JSC::ExecState*, JSC::ProgramExecutable*, JSC::JSObject**) XYZ/WebKit/Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1336:69 #19 0x7ff9b3b15a33 in JSC::ProgramExecutable::initializeGlobalProperties(JSC::VM&, JSC::ExecState*, JSC::JSScope*) XYZ/WebKit/Source/JavaScriptCore/runtime/ProgramExecutable.cpp:83:65 #20 0x7ff9b2814431 in JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) XYZ/WebKit/Source/JavaScriptCore/interpreter/Interpreter.cpp:874:36 #21 0x7ff9b34234e5 in JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) XYZ/WebKit/Source/JavaScriptCore/runtime/Completion.cpp:110:43 #22 0x4feada in runWithScripts(GlobalObject*, WTF::Vector<Script, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String const&, bool, bool, bool) XYZ/WebKit/Source/JavaScriptCore/jsc.cpp:2825:35 #23 0x4feada in runJSC(JSC::VM*, CommandLine) XYZ/WebKit/Source/JavaScriptCore/jsc.cpp:3102 #24 0x4fa755 in jscmain(int, char**) XYZ/WebKit/Source/JavaScriptCore/jsc.cpp:3154:14 #25 0x4fa389 in main XYZ/WebKit/Source/JavaScriptCore/jsc.cpp:2672:15 #26 0x7ff9ac57e82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f) #27 0x4249a8 in _start (/home/kamil/Fuzzing/webkit_jsc_240616/jsc+0x4249a8) AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: SEGV XYZ/WebKit/Source/WTF/wtf/Assertions.cpp:322:5 in WTFCrash ==27690==ABORTING Regards, Kamil Frankowicz
There are multiple issues here: 1. A release mode assertion that gets triggered for this script due to a parser bug. This reproduces in Safari without ASan, and is a regression in trunk. 2. The port you are using has an issue that causes invalid memory access when trying to cleanly crash. Let's track #1 in this bug, as it's a more generic issue.
<rdar://problem/29464028>
Looks like a simple enough fix. Shouldn't -Wswitch-enum have caught this before landing?
Can we move the UnaryOp flag up to 256? Otherwise, there are too many "keyword" tokens.
The token flags right now are something like this: R = right-associative bit T = unterminated error flag E = error flag I = binary operator allows 'in' P = binary operator precedence K = keyword flag U = unary operator flag 0b000000000RTEIIIIIIIIPPPPKUXXXXXX Since the bitfield is 32bits to begin with, do we lose much by allowing 127 keyword / unary op tokens?
Created attachment 295961 [details] Patch
If we don't want to land this, we can roll out https://trac.webkit.org/changeset/208933, but I don't think landing this should hurt anything
Comment on attachment 295961 [details] Patch Is the invariant here that the max JSTokenType needs to be < UnaryOpTokenFlag? If so, can you add a static_assert or regular ASSERT to that effect?
(In reply to comment #8) > Comment on attachment 295961 [details] > Patch > > Is the invariant here that the max JSTokenType needs to be < > UnaryOpTokenFlag? If so, can you add a static_assert or regular ASSERT to > that effect? there isn't really an invariant like that, as many of the JSTokenType enum values explicitly include the UnaryOpTokenFlag, binary precedence flags, or otherwise. Also, FYI, this bug reproduces with some other tokens on trunk, there are about 5 keyword tokens which wrap past the UnaryOpTokenFlag. I'm not sure what all of them are on the last STP (or Safari 9, if any), but in trunk, https://jsfiddle.net/rc8qL6m3/ crashes the tab in the same way. So, giving the extra bit should solve multiple problems there.
Comment on attachment 295961 [details] Patch Clearing flags on attachment: 295961 Committed r209293: <http://trac.webkit.org/changeset/209293>
All reviewed patches have been landed. Closing bug.
uppp, really should have fixed the wording in that ChangeLog. There's no extra "64 bits". If the message confuses anyone, hopefully they find this comment.
Committed r209295: <http://trac.webkit.org/changeset/209295>
Git freaked out and I accidentally committed with this patches changelog at the top. I fix that in https://trac.webkit.org/changeset/209296. It also looks like this patch's test fails on the bots though (https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/10867/steps/jscore-test/logs/stdio)
I'll take a look at that today when I get some time.
Reopening to attach new patch.
Created attachment 296053 [details] Patch
Attachment 296053 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:2588: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzing, fuzzing, fuzzing, fuzzing, fuzzing, fuzzing, fuzzing, fuzzing, fuzzing, fuzzing, fuzzing, fuzzing, fuzzing, fuzzing, fuzzing, fuzzing, fuzzing, fuzzing, fuzzing, fuzz test, malicious [changelog/unwantedsecurityterms] [3] ERROR: JSTests/ChangeLog:1025: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzz test, malicious [changelog/unwantedsecurityterms] [3] Total errors found: 2 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
So, I guess the main problem was just not reporting an error message in `parseMemberExpression` when "async" is followed by an identifier (it just marked the error as "this is probably an arrow function" and returned 0), which didn't work in unary expression contexts. But, some keywords do wrap around and get treated as unary ops when they shouldn't. That could be a problem, but I don't have a test case that crashes due to it. So, I can solve this crash without the ParserTokens.h changes, but maybe it makes sense to have them anyway.
Created attachment 296137 [details] Patch for relanding
Comment on attachment 296137 [details] Patch for relanding This patch fixes some problems with the previously landed one, including: 1. record error message in `parseMemberExpression` when an async arrow function is assumed (previously returned 0, causing the operatorString() path to be taken in a unary expr context) 2. bitfield comment is updated for correctness 3. changelog less confusing than it was previously Now, when I was first debugging this, I was seeing "async" treated as a unary op, and not "minus" as the unary op, so it seemed to make sense to fix the bitfield. It looks like it's not _strictly_ necessary to fix the bitfield, but is probably a good idea regardless. So, this is ready to go, but if preferred, the ParserTokens.h change should be safe to remove before landing, since the main fix is that one line in parseMemberExpression().
I'm confused about the state of this bug. The test added here has been failing since r209293. There is currently a "patch for relanding" up for review. Does this mean that all or parts of the initial patch got rolled out? Does the current patch fix the test failure? In any case, a prompt fix would be appreciated, as JSC test bots have been red for three days now.
Link to failing tests: https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20JSC%20(Tests)/builds/10897/steps/jscore-test/logs/stdio ** The following JSC stress test failures have been introduced: stress/bug-165091.js.default stress/bug-165091.js.dfg-eager stress/bug-165091.js.dfg-eager-no-cjit-validate stress/bug-165091.js.dfg-maximal-flush-validate-no-cjit stress/bug-165091.js.ftl-eager stress/bug-165091.js.ftl-eager-no-cjit stress/bug-165091.js.ftl-no-cjit-no-inline-validate stress/bug-165091.js.ftl-no-cjit-no-put-stack-validate stress/bug-165091.js.ftl-no-cjit-small-pool stress/bug-165091.js.ftl-no-cjit-validate-sampling-profiler stress/bug-165091.js.no-cjit-validate-phases stress/bug-165091.js.no-ftl stress/bug-165091.js.no-llint
(In reply to comment #22) > I'm confused about the state of this bug. > > The test added here has been failing since r209293. There is currently a > "patch for relanding" up for review. > > Does this mean that all or parts of the initial patch got rolled out? Does > the current patch fix the test failure? > > In any case, a prompt fix would be appreciated, as JSC test bots have been > red for three days now. You're right, this wasn't rolled out. I thought it had been. In any case, the fix for the test failure is in the latest patch.
Created attachment 296184 [details] Patch for landing Fixup to not call this a reland, and better describe what gets fixed here
Comment on attachment 296184 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=296184&action=review r=me > JSTests/stress/bug-165091.js:14 > -shouldThrowSyntaxError("0/-async J", "SyntaxError: Cannot parse member expression.") > \ No newline at end of file > +shouldThrowSyntaxError("0/-async J", "Unexpected identifier 'J'") Please add a new line at the end of the file. That will prevent these " \ No newline at end of file" text from showing up in the future.
Created attachment 296186 [details] Fix of test failure.
Comment on attachment 296186 [details] Fix of test failure. Rejecting attachment 296186 [details] from review queue. caitp@igalia.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights.
(In reply to comment #28) > caitp@igalia.com does not have reviewer permissions according to > http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/ > contributors.json. For future reference, this is how you handle a patch for landing via the cq: you can change the "Reviewed by" line in the patch to the reviewer who reviewed your patch, and just cq+ the patch.
(In reply to comment #29) > (In reply to comment #28) > > caitp@igalia.com does not have reviewer permissions according to > > http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/ > > contributors.json. > > For future reference, this is how you handle a patch for landing via the cq: > you can change the "Reviewed by" line in the patch to the reviewer who > reviewed your patch, and just cq+ the patch. I think I've been told this at least twice now, sorry. Most of my work is using different conventions in Chromium, so it's hard to commit this to memory. I'll stick a post-it note by my desk for next time.
Comment on attachment 295961 [details] Patch Un-obsoleting this patch because this was the original patch that was landed in r209293: <http://trac.webkit.org/changeset/209293>. The latest patch is only a delta to fix a bug in that original patch.
Comment on attachment 296186 [details] Fix of test failure. Clearing flags on attachment: 296186 Committed r209350: <http://trac.webkit.org/changeset/209350>