Bug 141070 – Crash in uninitialized deconstructing variable.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Han Choongwoo]

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.

[Han Choongwoo]

found with afl-fuzz

[Han Choongwoo]

Created attachment 245698 [details]
Patch

maybe enable profiling would be needed..
I don't know.

[Han Choongwoo]

Created attachment 245699 [details]
Patch

[Saam Barati]

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

[Saam Barati]

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

[Saam Barati]

(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};

[Saam Barati]

Created attachment 245734 [details]
patch

A patch like this should do the trick. It's a bit ugly though passing a flag into parseVarDeclarationList

[Geoffrey Garen]

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.

[Saam Barati]

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

[Saam Barati]

I forgot to link to the spec section I was referring to:

https://people.mozilla.org/~jorendorff/es6-draft.html#sec-variable-statement

[Saam Barati]

Created attachment 245787 [details]
patch

[Saam Barati]

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*

[Michael Saboff]

Comment on attachment 245787 [details]
patch

r=me with your ChangeLog change.

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/179682