Bug 141070 - Crash in uninitialized deconstructing variable.
Summary: Crash in uninitialized deconstructing variable.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Critical
Assignee: Saam Barati
URL:
Keywords:
Depends on: 135545
Blocks:
  Show dependency treegraph
 
Reported: 2015-01-29 23:19 PST by Han Choongwoo
Modified: 2015-02-05 00:59 PST (History)
4 users (show)

See Also:


Attachments
Patch (8.80 KB, patch)
2015-01-29 23:25 PST, Han Choongwoo
no flags Details | Formatted Diff | Diff
Patch (8.86 KB, patch)
2015-01-29 23:35 PST, Han Choongwoo
no flags Details | Formatted Diff | Diff
patch (3.87 KB, patch)
2015-01-30 13:03 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (8.46 KB, patch)
2015-01-31 10:51 PST, Saam Barati
msaboff: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Han Choongwoo 2015-01-29 23:19:35 PST
I think this bug is related with https://bugs.webkit.org/show_bug.cgi?id=135545
variable declarations with no initializer, something like 'var {A};' or 'var [A];', make crash.
these declarations also need AST nodes.
Comment 1 Han Choongwoo 2015-01-29 23:23:08 PST
found with afl-fuzz
Comment 2 Han Choongwoo 2015-01-29 23:25:05 PST
Created attachment 245698 [details]
Patch

maybe enable profiling would be needed..
I don't know.
Comment 3 Han Choongwoo 2015-01-29 23:35:16 PST
Created attachment 245699 [details]
Patch
Comment 4 Saam Barati 2015-01-30 10:42:31 PST
(In reply to comment #0)
> I think this bug is related with
> https://bugs.webkit.org/show_bug.cgi?id=135545
> variable declarations with no initializer, something like 'var {A};' or 'var
> [A];', make crash.
> these declarations also need AST nodes.

Is it valid ES6 syntax to do something like:

var {foo};
foo = 20;

?

I'm not sure what the spec says for things like this. Looking it up now.
Comment 5 Saam Barati 2015-01-30 11:26:44 PST
(In reply to comment #0)
> I think this bug is related with
> https://bugs.webkit.org/show_bug.cgi?id=135545
> variable declarations with no initializer, something like 'var {A};' or 'var
> [A];', make crash.
> these declarations also need AST nodes.

From reading the spec, I don't think this is valid. If you refer to section 13.2.2, it says that all BindingPattern nodes inside a variable declaration must have an accompanying initializer. That said, JSC should definitely not crash on this. We should raise a parsing error.

I think this can be fixed inside Parser::parseVarDeclarationList on line 489+ by raising a parse error for any deconstruction pattern node that does not contain a rhs.
Comment 6 Saam Barati 2015-01-30 12:08:49 PST
(In reply to comment #5)
> (In reply to comment #0)
> > I think this bug is related with
> > https://bugs.webkit.org/show_bug.cgi?id=135545
> > variable declarations with no initializer, something like 'var {A};' or 'var
> > [A];', make crash.
> > these declarations also need AST nodes.
> 
> From reading the spec, I don't think this is valid. If you refer to section
> 13.2.2, it says that all BindingPattern nodes inside a variable declaration
> must have an accompanying initializer. That said, JSC should definitely not
> crash on this. We should raise a parsing error.
> 
> I think this can be fixed inside Parser::parseVarDeclarationList on line
> 489+ by raising a parse error for any deconstruction pattern node that does
> not contain a rhs.

It's actually not this easy. parseVarDeclarationList is used both in for statements and in variable declaration statements. This is somewhat unfortunate, anyways, it should be legal to do:
for (var {x} of arr) ...
but not:
var {x};
Comment 7 Saam Barati 2015-01-30 13:03:28 PST
Created attachment 245734 [details]
patch

A patch like this should do the trick. It's a bit ugly though passing a flag into parseVarDeclarationList
Comment 8 Geoffrey Garen 2015-01-30 14:08:48 PST
Comment on attachment 245734 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=245734&action=review

> Source/JavaScriptCore/parser/Parser.cpp:381
> +    TreeExpression varDecls = parseVarDeclarationList(context, scratch, scratch1, scratch2, scratch3, scratch3, scratch3, false);

When we write code like this, we either use an enumeration or we explicitly assign the boolean value to a named local variable. Otherwise, it's hard to know what the parameter means.
Comment 9 Saam Barati 2015-01-30 14:31:35 PST
(In reply to comment #8)
> Comment on attachment 245734 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245734&action=review
> 
> > Source/JavaScriptCore/parser/Parser.cpp:381
> > +    TreeExpression varDecls = parseVarDeclarationList(context, scratch, scratch1, scratch2, scratch3, scratch3, scratch3, false);
> 
> When we write code like this, we either use an enumeration or we explicitly
> assign the boolean value to a named local variable. Otherwise, it's hard to
> know what the parameter means.

Yeah, agreed. I'll make it into an enum, it seems cleanest that way.
Comment 10 Saam Barati 2015-01-31 10:36:06 PST
I forgot to link to the spec section I was referring to:

https://people.mozilla.org/~jorendorff/es6-draft.html#sec-variable-statement
Comment 11 Saam Barati 2015-01-31 10:51:05 PST
Created attachment 245787 [details]
patch
Comment 12 Saam Barati 2015-02-03 13:15:11 PST
Comment on attachment 245787 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=245787&action=review

> Source/JavaScriptCore/ChangeLog:10
> +        statement, the assignment must have also right hand side value.

Typo:
*the assignment must also have a right hand side value*
Comment 13 Michael Saboff 2015-02-03 14:11:50 PST
Comment on attachment 245787 [details]
patch

r=me with your ChangeLog change.
Comment 14 Saam Barati 2015-02-05 00:59:45 PST
landed in:
http://trac.webkit.org/changeset/179682