1350464 - Crash [@ js::frontend::TokenStream::getTokenInternal]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The following testcase crashes on mozilla-central revision 4c987b7ed54a (build with --enable-debug, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

oomTest(function() {
    try {



























































































































    } catch(e) {
        ;
    }
})

Backtrace:

#0  0x0000000000d3ec28 in js::frontend::TokenStream::getTokenInternal (this=0x7fffa1836558, ttp=0x7fffa18359ac, modifier=modifier@entry=js::frontend::Token::Operand) at js/src/frontend/TokenStream.cpp:1942
#1  0x00000000004a0f0a in js::frontend::TokenStream::peekToken (this=this@entry=0x7fffa1836558, ttp=ttp@entry=0x7fffa18359ac, modifier=js::frontend::Token::Operand, modifier=js::frontend::Token::Operand) at js/src/frontend/TokenStream.h:603
#2  0x00000000004d58c3 in js::frontend::Parser<js::frontend::FullParseHandler>::statementList (this=this@entry=0x7fffa1836540, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName) at js/src/frontend/Parser.cpp:4029
#3  0x00000000004d60f1 in js::frontend::Parser<js::frontend::FullParseHandler>::catchBlockStatement (this=this@entry=0x7fffa1836540, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, catchParamScope=...) at js/src/frontend/Parser.cpp:6918
#4  0x00000000004d679c in js::frontend::Parser<js::frontend::FullParseHandler>::tryStatement (this=this@entry=0x7fffa1836540, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName) at js/src/frontend/Parser.cpp:6841
#5  0x00000000004d545b in js::frontend::Parser<js::frontend::FullParseHandler>::statementListItem (this=this@entry=0x7fffa1836540, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, canHaveDirectives=<optimized out>) at js/src/frontend/Parser.cpp:7580
/snip

For detailed crash information, see attachment.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Detailed Crash Information

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: testcase

I have attached the testcase again in case the whitespaces (which are required) do not show up properly.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Whoops, still getting used to the new b.m.o interface. It might be related to bug 1326454 as the possible regressor, so pushing over to :Waldo to take a look.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/65f852c28e71
user:        Jeff Walden
date:        Wed Jan 04 14:45:37 2017 -0800
summary:     Bug 1326454 - Make TokenStream::updateLineInfoForEOL fallible, and immediately handle errors when it fails instead of temporarily deferring such handling.  r=arai

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Don't try to poison not-yet-allocated structures when an error occurs during parsing

Woops.

Comment 6

[Tooru Fujisawa [:arai]]

Comment on attachment 8851193 [details] [diff] [review]
Don't try to poison not-yet-allocated structures when an error occurs during parsing

Review of attachment 8851193 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/TokenStream.cpp
@@ +1939,5 @@
>  
>    error:
> +    // We didn't get a token, so don't set |flags.isDirtyLine|.  And don't
> +    // poison any of |*tp|: if we haven't allocated a token, |tp| could be
> +    // uninitialized.

error is reachable both from before and after |tp = newToken(-1);|,
it looks like we should set |tp = nullptr| at the beginning and poison it when tp is non-null.

Comment 7

[Tooru Fujisawa [:arai]]

Comment on attachment 8851193 [details] [diff] [review]
Don't try to poison not-yet-allocated structures when an error occurs during parsing

Review of attachment 8851193 [details] [diff] [review]:
-----------------------------------------------------------------

discussed in IRC, and it should be okay not to poison it there

Comment 8

[Christian Holler (:decoder)]

Marking as fuzzblocker due to the amount of crashes I see with this signature and s-s because several of them have a non-zero crash address.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/06e1e7de67bc

Comment 10

[Jeff Walden [:Waldo]]

Woops, didn't realize this was (properly) marked sensitive.  Updating with the proper rating -- and I guess per my usual practice I should have landed the fix, then a week or so later landed the test, as a nicety to nightly users -- but this was only ever in nightly, so no forgotten intent to be careful about landing the fix for this.