Summary: | Crash in uninitialized deconstructing variable. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Han Choongwoo <cwhan.tunz> | ||||||||||
Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Critical | CC: | fpizlo, ggaren, oliver, saam | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 135545 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Han Choongwoo
2015-01-29 23:19:35 PST
found with afl-fuzz Created attachment 245698 [details]
Patch
maybe enable profiling would be needed..
I don't know.
Created attachment 245699 [details]
Patch
(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. (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. (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}; 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 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. (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. I forgot to link to the spec section I was referring to: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-variable-statement Created attachment 245787 [details]
patch
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 on attachment 245787 [details]
patch
r=me with your ChangeLog change.
landed in: http://trac.webkit.org/changeset/179682 |