Bug 165091 - REGRESSION: Reproducible crash in operatorString() on invalid code with async
Summary: REGRESSION: Reproducible crash in operatorString() on invalid code with async
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caitlin Potter (:caitp)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-11-28 09:49 PST by Kamil Frankowicz
Modified: 2016-12-05 14:20 PST (History)
12 users (show)

See Also:


Attachments
POC to trigger out of bounds read (jsc) (10 bytes, application/javascript)
2016-11-28 09:49 PST, Kamil Frankowicz
no flags Details
Patch (3.45 KB, patch)
2016-12-02 10:41 PST, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Patch (1021.86 KB, patch)
2016-12-03 10:29 PST, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Patch for relanding (3.27 KB, patch)
2016-12-05 07:22 PST, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Patch for landing (3.21 KB, patch)
2016-12-05 13:27 PST, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Fix of test failure. (3.22 KB, patch)
2016-12-05 13:53 PST, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kamil Frankowicz 2016-11-28 09:49:30 PST
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
Comment 1 Alexey Proskuryakov 2016-12-01 11:10:39 PST
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.
Comment 2 Radar WebKit Bug Importer 2016-12-01 12:07:35 PST
<rdar://problem/29464028>
Comment 3 Caitlin Potter (:caitp) 2016-12-02 10:01:42 PST
Looks like a simple enough fix. Shouldn't -Wswitch-enum have caught this before landing?
Comment 4 Caitlin Potter (:caitp) 2016-12-02 10:12:53 PST
Can we move the UnaryOp flag up to 256? Otherwise, there are too many "keyword" tokens.
Comment 5 Caitlin Potter (:caitp) 2016-12-02 10:26:34 PST
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?
Comment 6 Caitlin Potter (:caitp) 2016-12-02 10:41:17 PST
Created attachment 295961 [details]
Patch
Comment 7 Caitlin Potter (:caitp) 2016-12-02 11:33:12 PST
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 8 Geoffrey Garen 2016-12-02 11:37:34 PST
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?
Comment 9 Caitlin Potter (:caitp) 2016-12-02 11:50:56 PST
(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 10 WebKit Commit Bot 2016-12-02 19:40:05 PST
Comment on attachment 295961 [details]
Patch

Clearing flags on attachment: 295961

Committed r209293: <http://trac.webkit.org/changeset/209293>
Comment 11 WebKit Commit Bot 2016-12-02 19:40:09 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Caitlin Potter (:caitp) 2016-12-02 19:49:06 PST
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.
Comment 13 Keith Miller 2016-12-02 22:41:02 PST
Committed r209295: <http://trac.webkit.org/changeset/209295>
Comment 14 Keith Miller 2016-12-03 08:34:23 PST
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)
Comment 15 Caitlin Potter (:caitp) 2016-12-03 09:00:50 PST
I'll take a look at that today when I get some time.
Comment 16 Caitlin Potter (:caitp) 2016-12-03 10:29:38 PST
Reopening to attach new patch.
Comment 17 Caitlin Potter (:caitp) 2016-12-03 10:29:49 PST
Created attachment 296053 [details]
Patch
Comment 18 WebKit Commit Bot 2016-12-03 10:30:50 PST
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.
Comment 19 Caitlin Potter (:caitp) 2016-12-03 10:42:39 PST
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.
Comment 20 Caitlin Potter (:caitp) 2016-12-05 07:22:01 PST
Created attachment 296137 [details]
Patch for relanding
Comment 21 Caitlin Potter (:caitp) 2016-12-05 08:10:12 PST
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().
Comment 22 Alexey Proskuryakov 2016-12-05 13:15:26 PST
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.
Comment 23 Alexey Proskuryakov 2016-12-05 13:16:06 PST
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
Comment 24 Caitlin Potter (:caitp) 2016-12-05 13:21:37 PST
(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.
Comment 25 Caitlin Potter (:caitp) 2016-12-05 13:27:47 PST
Created attachment 296184 [details]
Patch for landing

Fixup to not call this a reland, and better describe what gets fixed here
Comment 26 Mark Lam 2016-12-05 13:50:49 PST
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.
Comment 27 Caitlin Potter (:caitp) 2016-12-05 13:53:46 PST
Created attachment 296186 [details]
Fix of test failure.
Comment 28 WebKit Commit Bot 2016-12-05 13:54:49 PST
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.
Comment 29 Mark Lam 2016-12-05 13:58:09 PST
(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.
Comment 30 Caitlin Potter (:caitp) 2016-12-05 14:00:27 PST
(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 31 Mark Lam 2016-12-05 14:05:50 PST
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 32 WebKit Commit Bot 2016-12-05 14:20:34 PST
Comment on attachment 296186 [details]
Fix of test failure.

Clearing flags on attachment: 296186

Committed r209350: <http://trac.webkit.org/changeset/209350>
Comment 33 WebKit Commit Bot 2016-12-05 14:20:39 PST
All reviewed patches have been landed.  Closing bug.