1299519 - Assertion failure: loc->kind() != NameLocation::Kind::FrameSlot, at BytecodeEmitter.cpp

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

Configure flags:
  --enable-debug --disable-optimize --enable-warnings-as-errors --enable-nspr-build

Code:
  class C {
  }
  class D extends C {
    constructor() {
      var [a=(for (x of [1, 2, 3]) x),, ...c] = [];
    }
  }

Actual result:
  Assertion failure: loc->kind() != NameLocation::Kind::FrameSlot, at js/src/frontend/BytecodeEmitter.cpp:784

https://dxr.mozilla.org/mozilla-central/rev/506facea63169a29e04eb140663da1730052db64/js/src/frontend/BytecodeEmitter.cpp#784
>     // Each script has its own frame. A free name that is accessed
>     // from an inner script must not be a frame slot access. If this
>     // assertion is hit, it is a bug in the free name analysis in the
>     // parser.
>     MOZ_ASSERT_IF(!inCurrentScript, loc->kind() != NameLocation::Kind::FrameSlot);

regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cd4ed9909dc9da71e07af183e4293d95ed5e9160&tochange=8f6cbb5a8f4a748cd9fd104e58e840ca6e86b253

before the range, it hits different assertion:
  Assertion failure: !done(), at js/src/jsscript.h:2181

https://hg.mozilla.org/mozilla-central/file/181336fdda66/js/src/jsscript.h#l2181
>     const Binding* operator->() const { MOZ_ASSERT(!done()); return &bindingArray_[i_]; }


Perhaps something was changed by bug 1263355, but it would be pre-existing bug.


marking s-s just in case.

Comment 1

[Tooru Fujisawa [:arai]]

actually, the destructuring was unrelated (was testing bug 1184922 patch).

simplified code:
  class C {
  }
  class D extends C {
    constructor() {
      var a=(for (x of []) 1);
    }
  }

Comment 2

[Jeff Walden [:Waldo]]

7645	Parser<ParseHandler>::generatorComprehensionLambda(unsigned begin)
7646	{
...
7688	    genFunbox->anyCxFlags = outerpc->sc()->anyCxFlags;
7689	    if (outerpc->isFunctionBox())
7690	        genFunbox->funCxFlags = outerpc->functionBox()->funCxFlags;

Turns out that exactly copying in the outer funCxFlags, including funCxFlags.isDerivedClassConstructor, is not quite a good idea!  :-)  So basically, a generator comprehension lambda inside a derived class constructor is going to Die.

Patch anon.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Fugly patch

This is the right fix, IMO, but the paint job is a solid puce.  Feel free to suggest anything at all better.

Comment 4

[Shu-yu Guo [:shu]]

Comment on attachment 8786947 [details] [diff] [review]
Fugly patch

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

Yikes, good catch. Could use a test.

::: js/src/frontend/Parser.cpp
@@ +7688,5 @@
>      genFunbox->anyCxFlags = outerpc->sc()->anyCxFlags;
> +    if (outerpc->isFunctionBox()) {
> +        genFunbox->funCxFlags =
> +            outerpc->functionBox()->flagsForNestedGeneratorComprehensionLambda();
> +    }

I'm 95% sure this bit (and thus the new functions) can be deleted. The only thing it propagates is mightAliasLocals, which no longer does anything. It's used to propagate itself in BCE, and I don't see any legitimate uses of it anywhere.

Could you try deleting this, that flag, and see if anything breaks?

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Test

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Trunk patch

This does seem to check out on jstests/jit-tests.  Haven't tryservered because we're off the rails in uncertain ways with uncertain consequences, but yeah, I suspect this pans out.

This probably needs branch backporting, so the test and this patch are probably not landable til we have those in hand.  And I don't necessarily know if the same not-used analysis we've applied on trunk applies on branches, so I'd rather have all my ducks in a row before landing or likely even requesting approval.

Comment 7

[Shu-yu Guo [:shu]]

Comment on attachment 8786996 [details] [diff] [review]
Trunk patch

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

gr8

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Aurora/Beta/ESR45 backport

We still use mightAliasLocals in BytecodeEmitter::tryConvertFreeName before bug 1263355, so we have to fugly on branches.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 8786996 [details] [diff] [review]
Trunk patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

No idea.  There are eight different flags whose mis-calculation could cause things to go awry in exciting ways.  It is not worth our time to patiently track through the ramifications of all eight possible miscalculations and just how bad it could get.

> Do comments in the patch, the check-in comment, or tests included
> in the patch paint a bulls-eye on the security problem?

No.  Tests are separate and will land at a remove in time.

> Which older supported branches are affected by this flaw?
> If not all supported branches, which bug introduced the flaw?

Everything, this is one from the vaults.

> Do you have backports for the affected branches? If not, how
> different, hard to create, and risky will they be?

Backports created -- somewhat different but not horrifically so.

> How likely is this patch to cause regressions; how much
> testing does it need?

Tryservering should be adequate.  (I haven't done so yet so as not to reveal the issue, will do before landing once approval is in.)

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 8787014 [details] [diff] [review]
Aurora/Beta/ESR45 backport

[Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:

Out-of-bounds memory accesses and subsequent effects on control flow, plus whatever mayhem results if the *other* flags at issue here are wrong, is at least prudentially sec-critical.

> User impact if declined:

Out-of-bounds memory accesses, undetermined mayhem.

> Fix Landed on Version:

Still to land, getting in the queue early because uplift approaches.

> Risk to taking this patch (and alternatives if risky):

Lowish.  No alternatives to taking it, tho.

> String or UUID changes made by this patch: N/A

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/regressing bug #]: unthinkably ancient
[User impact if declined]: out-of-bounds memory accesses, undetermined mayhem
[Describe test coverage new/current, TreeHerder]: test in bug, every patch here makes the test pass
[Risks and why]: see above
[String/UUID change made/needed]: N/A

Comment 11

[Daniel Veditz [:dveditz]]

I don't know if we have time to get this safely landed in 49 but tracking until we know for sure we can't.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

This is well too late. Final beta was made today. I'll give sec-approval but this shouldn't land until September 20, two weeks into the next cycle.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8787014 [details] [diff] [review]
Aurora/Beta/ESR45 backport

sec-crit, Aurora50+

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Jeff, this hasn't landed on Nightly yet. Is it supposed to land on m-c, m-a at the same time?

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Ok, just saw the checkin-on-Sept20 whiteboard tag.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8787014 [details] [diff] [review]
Aurora/Beta/ESR45 backport

Hi Dan, (one more) should this land on 9/27 given that Fx49 release go-live was pushed out a by a week?

Comment 17

[Daniel Veditz [:dveditz]]

This can land any time now

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7340398c68a8

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7340398c68a8

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bc1c04505a77

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/da90ddb4a187

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/fc055950b6b8

Comment 23

[Kamil Jozwiak [:kjozwiak]]

Reproduced the assertion from comment#0 using the simplified POC in comment#1 with the following js build:
* fx51.0a1, changeset: 506facea63169a29e04eb140663da1730052db64

kjozwiak@ubuntu:~/mozcode/mozilla-central/js/src/build_OPT.OBJ/dist/bin$ ./js ~/Desktop/poc.js
Assertion failure: loc->kind() != NameLocation::Kind::FrameSlot, at /home/kjozwiak/mozcode/mozilla-central/js/src/frontend/BytecodeEmitter.cpp:784

Went through verification using the POC in comment#1 with the following builds:

* fx52.0a1, changeset: aea5b4c3d165 - PASSED
* fx51.0a2, changeset: 71a57bc90cf2 - PASSED
* fx50.0b9, changeset: 24b8f08f7756 - PASSED
* fx45.5.0esr, changeset: 66f2c78e016a - PASSED

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50b8043a629c
Add a test for generator comprehensions inside derived class constructors.  r=shu

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/50b8043a629c