Closed Bug 1299519 Opened 5 years ago Closed 5 years ago

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


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox48 --- wontfix
firefox49 + wontfix
firefox-esr45 50+ verified
firefox50 + verified
firefox51 + verified
firefox52 --- verified


(Reporter: arai, Assigned: Waldo)


(5 keywords, Whiteboard: [post-critsmash-triage][adv-main50+][adv-esr45.5+])


(3 files, 1 obsolete file)

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

  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
>     // 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:

before the range, it hits different assertion:
  Assertion failure: !done(), at js/src/jsscript.h:2181
>     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.
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);
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.
Assignee: nobody → jwalden+bmo
Attached patch Fugly patch (obsolete) — Splinter Review
This is the right fix, IMO, but the paint job is a solid puce.  Feel free to suggest anything at all better.
Attachment #8786947 - Flags: review?(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?
Attachment #8786947 - Flags: review?(shu)
Attached patch TestSplinter Review
Attachment #8786995 - Flags: review?(shu)
Attached patch Trunk patchSplinter Review
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.
Attachment #8786996 - Flags: review?(shu)
Attachment #8786947 - Attachment is obsolete: true
Attachment #8786995 - Flags: review?(shu) → review+
Comment on attachment 8786996 [details] [diff] [review]
Trunk patch

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

Attachment #8786996 - Flags: review?(shu) → review+
We still use mightAliasLocals in BytecodeEmitter::tryConvertFreeName before bug 1263355, so we have to fugly on branches.
Attachment #8787014 - Flags: review?(shu)
Attachment #8786996 - Attachment is obsolete: true
Attachment #8786996 - Attachment description: Updated for removal suggestion → Trunk patch
Attachment #8786996 - Attachment is obsolete: false
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.)
Attachment #8786996 - Flags: sec-approval?
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 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
Attachment #8787014 - Flags: approval-mozilla-esr45?
Attachment #8787014 - Flags: approval-mozilla-beta?
Attachment #8787014 - Flags: approval-mozilla-aurora?
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.
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.
Whiteboard: [checkin on 9/20]
Attachment #8786996 - Flags: sec-approval? → sec-approval+
Attachment #8787014 - Flags: review?(shu) → review+
Attachment #8787014 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Group: core-security → javascript-core-security
Comment on attachment 8787014 [details] [diff] [review]
Aurora/Beta/ESR45 backport

sec-crit, Aurora50+
Attachment #8787014 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi Jeff, this hasn't landed on Nightly yet. Is it supposed to land on m-c, m-a at the same time?
Flags: needinfo?(jwalden+bmo)
Ok, just saw the checkin-on-Sept20 whiteboard tag.
Flags: needinfo?(jwalden+bmo)
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?
Flags: needinfo?(dveditz)
Attachment #8787014 - Flags: approval-mozilla-esr45?
Attachment #8787014 - Flags: approval-mozilla-esr45+
Attachment #8787014 - Flags: approval-mozilla-beta-
Attachment #8787014 - Flags: approval-mozilla-beta+
This can land any time now
Flags: needinfo?(dveditz)
Whiteboard: [checkin on 9/20]
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Group: javascript-core-security → core-security-release
QA Contact: kjozwiak
Whiteboard: [post-critsmash-triage]
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
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main50+][adv-esr45.5+]
Group: core-security-release
Pushed by
Add a test for generator comprehensions inside derived class constructors.  r=shu
You need to log in before you can comment on or make changes to this bug.