This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

CCing random folks in case I don't have time to look at this today

Oh noes, the repro case is a protobuf. :(

Need to figure out how to repro...

The repro case is:

( function *  ( [ name = [ foo ] = eval ( foo ) ] ) { as } )

And indeed running it on debug mode triggers the DCHECK.

Yay, this was the first real bug found by my new parser fuzzer!!!

Simpler repro:

(function( [ name = [ foo ] = eval ("") ] ) { })

Slightly simpler (in that it has fewer catch scopes):

(function( { name = [ foo ] = eval ("") } ) { })

I have a pretty good handle on this, I think (though not sure yet what solution I want to use to fix it). The problem is that by the time we get around to rewriting the "[ foo ] = eval()" expression, we've lost track of the varblock scope that's supposed to be surrounding it. 

To expand a little further: the scope we queue with the assignment is the function scope, and so that's the one we use during pattern rewriting. This results in the generated do-expression having the wrong scope chain, as we created an inner varblock scope for the initializer of "name" back during parameter destructuring.

It's interesting to see the divergence in behavior across engines (if we run this in a release build, where the dcheck doesn't take effect):

```
$ eshost -e '(function( { a = [b] = eval("var x = 5; [42]") } ) { return x})({})'
#### v8
ReferenceError: x is not defined

#### chakra
SyntaxError: 'eval' is not allowed in the default initializer

#### jsc
5

#### spidermonkey
ReferenceError: x is not defined
```

v8 and spidermonkey get the right answer; JSC seems to not be creating the required extra scope for the parameter. And Chakra doesn't even allow eval in this case!

Friendly ping; any updates on this?

My work so far on this was to aim for a spec change that would make it relatively easy to fix. As-is, fixing this while maintaining the current behavior would be quite hard.

I'll try to figure out what to do with this this week.

Potential fix up at https://chromium-review.googlesource.com/c/v8/v8/+/767666

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/082009fc3deca927755e40e2187c244f910e9abe

commit 082009fc3deca927755e40e2187c244f910e9abe
Author: Adam Klein <adamk@chromium.org>
Date: Tue Nov 14 20:30:14 2017

[parser] RewritableExpressions should keep track of their Scope directly

Previously, the Parser stored a Scope alongside a RewritableExpression
for each potential destructuring assignment. This Scope was later used
during rewriting to set the correct context for the rewriting. But this
approach failed if a new Scope was inserted into the Scope chain between
the time the assignment was parsed and when it was rewritten.

By storing the Scope directly in RewritableExpression,
ReparentExpressionScopes() is able to appropriately re-scope such
expressions prior to their rewriting.

Bug:  chromium:779457 
Change-Id: Ieb429a3da841f76d5798610af59da4fccb000652
Reviewed-on: https://chromium-review.googlesource.com/767666
Commit-Queue: Adam Klein <adamk@chromium.org>
Reviewed-by: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49368}
[modify] https://crrev.com/082009fc3deca927755e40e2187c244f910e9abe/src/ast/ast.h
[modify] https://crrev.com/082009fc3deca927755e40e2187c244f910e9abe/src/parsing/expression-scope-reparenter.cc
[modify] https://crrev.com/082009fc3deca927755e40e2187c244f910e9abe/src/parsing/parser-base.h
[modify] https://crrev.com/082009fc3deca927755e40e2187c244f910e9abe/src/parsing/parser.cc
[modify] https://crrev.com/082009fc3deca927755e40e2187c244f910e9abe/src/parsing/parser.h
[modify] https://crrev.com/082009fc3deca927755e40e2187c244f910e9abe/src/parsing/pattern-rewriter.cc
[modify] https://crrev.com/082009fc3deca927755e40e2187c244f910e9abe/src/parsing/preparser.h
[add] https://crrev.com/082009fc3deca927755e40e2187c244f910e9abe/test/mjsunit/regress/regress-crbug-779457.js

ClusterFuzz has detected this issue as fixed in range 516578:516592.

Detailed report: https://clusterfuzz.com/testcase?key=6658585231884288

Fuzzer: libFuzzer_javascript_parser_proto_fuzzer
Job Type: libfuzzer_chrome_asan_debug
Platform Id: linux

Crash Type: DCHECK failure
Crash Address: 
Crash State:
  outer_scope_ == scope->outer_scope() in bytecode-generator.cc
  CurrentScope
  v8::internal::interpreter::BytecodeGenerator::VisitInScope
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan_debug&range=510082:510099
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan_debug&range=516578:516592

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6658585231884288

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

ClusterFuzz testcase 6658585231884288 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

We should consider merging this to M63, as it is a security fix. But it's also existed for quite awhile.

+awhalley@ for M63 merge review

This bug requires manual review: M63 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

 adamk@ - clusterfuzz thinks this regressed in 64, is that incorrect?

The revision range is incorrect, as it's blaming marja's https://chromium.googlesource.com/chromium/src/+/5bfe933832d938572a4b278233ae1f3a805394d9, which added the fuzzer that generated this case. I've confirmed locally that the test cases fail on M63 (and M62).

Thanks adamk@! Removing release bock since it's not a regression. No need to race this into the first 63 stable.

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot