Bug 165091 – REGRESSION: Reproducible crash in operatorString() on invalid code with async

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Kamil Frankowicz]

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

[Alexey Proskuryakov]

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.

[Radar WebKit Bug Importer]

<rdar://problem/29464028>

[Caitlin Potter (:caitp)]

Looks like a simple enough fix. Shouldn't -Wswitch-enum have caught this before landing?

[Caitlin Potter (:caitp)]

Can we move the UnaryOp flag up to 256? Otherwise, there are too many "keyword" tokens.

[Caitlin Potter (:caitp)]

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?

[Caitlin Potter (:caitp)]

Created attachment 295961 [details]
Patch

[Caitlin Potter (:caitp)]

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

[Geoffrey Garen]

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?

[Caitlin Potter (:caitp)]

(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.

[WebKit Commit Bot]

Comment on attachment 295961 [details]
Patch

Clearing flags on attachment: 295961

Committed r209293: <http://trac.webkit.org/changeset/209293>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Caitlin Potter (:caitp)]

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.

[Keith Miller]

Committed r209295: <http://trac.webkit.org/changeset/209295>

[Keith Miller]

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)

[Caitlin Potter (:caitp)]

I'll take a look at that today when I get some time.

[Caitlin Potter (:caitp)]

Reopening to attach new patch.

[Caitlin Potter (:caitp)]

Created attachment 296053 [details]
Patch

[WebKit Commit Bot]

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.

[Caitlin Potter (:caitp)]

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.

[Caitlin Potter (:caitp)]

Created attachment 296137 [details]
Patch for relanding

[Caitlin Potter (:caitp)]

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().

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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

[Caitlin Potter (:caitp)]

(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.

[Caitlin Potter (:caitp)]

Created attachment 296184 [details]
Patch for landing

Fixup to not call this a reland, and better describe what gets fixed here

[Mark Lam]

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.

[Caitlin Potter (:caitp)]

Created attachment 296186 [details]
Fix of test failure.

[WebKit Commit Bot]

Comment on attachment 296186 [details]
Fix of test failure.

Rejecting attachment 296186 [details] from review queue.

[e-mail] 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.

[Mark Lam]

(In reply to comment #28)
> [e-mail] 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.

[Caitlin Potter (:caitp)]

(In reply to comment #29)
> (In reply to comment #28)
> > [e-mail] 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.

[Mark Lam]

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.

[WebKit Commit Bot]

Comment on attachment 296186 [details]
Fix of test failure.

Clearing flags on attachment: 296186

Committed r209350: <http://trac.webkit.org/changeset/209350>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.