1075546 - Assertion failure: entry_ == makeIndex(clasp, key, kind), at vm/Runtime.h

Status: VERIFIED FIXED

(Core :: JavaScript: GC, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

for (var i = 0; i < 200; ++i) {
    Object.getOwnPropertyNames(undefined + "");
}
function p(s) {
    for (var i = 0; i < s.length; i++) {
        s.charCodeAt(i);
    }
}
function m(f) {
    var a = [];
    for (var j = 0; j < 700; ++j) {
        try {
            f()
        } catch (e) {
            a.push(e.toString());
        }
    }
    p(uneval(a));
}
f = Function("\
    function f() {\
        functionf\n{}\
    }\
    m(f);\
");
f();
f();
for (var x = 0; x < 99; x++) {
    newGlobal()
}

asserts the following js debug shell on m-c changeset 6c824fbb73e8 with --no-asmjs --no-baseline --no-ion --no-threads at Assertion failure: entry_ == makeIndex(clasp, key, kind), at vm/Runtime.h.

https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-10-01-mozilla-central-debug/jsshell-mac64.zip

=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20140930050920" and the hash "1de7e7a7ad8d".
The "bad" changeset has the timestamp "20140930052624" and the hash "f33f7dc50aa9".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1de7e7a7ad8d&tochange=f33f7dc50aa9

Jon, is bug 1073577 a possible regressor? (Setting this as s-s because bug 1073577 is s-s.)

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

There are no nice stacks available for js shells from tinderbox-builds.

Comment 2

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This does not reproduce for locally compiled builds but seems to reproduce readily with tinderbox-builds ones.

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Gary Kwong [:gkw] [:nth10sd] GMT-4 Sep 29 - Oct 3 from comment #0)
Bug 1073577 added the assertion, but this may be a pre-existing problem.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug1075546

Repros fine for me with the attached testcase.

The problem was the fix for bug 1073577 was incomplete.  When recording the current GC number, it's important to record it *before* a GC can happen :)

With this patch, the gc number is recorded only if the cache lookup fails.  If it succeeds, there are two cases - either we create a new object successfully and return, or we fail and GC.  In the latter case we just leave gcNumber initialized to zero, which means the later test for GC will always fail.

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 8499493 [details] [diff] [review]
bug1075546

[Security approval request comment]

This is a followup to fix an issue with the fix in bug 1073577, so the details here are mostly the same.

How easily could an exploit be constructed based on the patch?

Difficult.

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

The contents of the patch indicate an issue with GCs occurring in specific places, but I don't think it's easy to take advantage of that.

Which older supported branches are affected by this flaw?

As per bug 1073577, everything back to FF 32.

If not all supported branches, which bug introduced the flaw?

Generational GC enabled on desktop in bug 619558.

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

Backporting should be combined with bug 1073577, the patch is easy to create and should apply without problems.

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

Should be low risk, again it would be a good idea to let it sit on central for a week first.

Comment 7

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

Comment on attachment 8499493 [details] [diff] [review]
bug1075546

This is too late to make it into 33. We're building soon for the release build. We should get this on Aurora though. Can you make and nominate an Aurora patch?

Comment 8

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

What is the security severity on this? bug 1073577 is a sec-audit, though I don't know if that's actually correct.

Comment 9

[Jon Coppeard (:jonco)]

(In reply to Al Billings [:abillings] from comment #8)
I've nominated the bug 1073577 for aurora/beta uplift.  The patch here will apply cleanly after that one.  I'm waiting on sec-approval to land this though (my guess at a security rating is for the original bug is sec-high).

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Jon, please go ahead and do the Aurora approval request when you push this to inbound so we don't wait another day to cycle around.

Comment 11

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4007aeccec79

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4007aeccec79

Comment 13

[Jon Coppeard (:jonco)]

Comment on attachment 8499493 [details] [diff] [review]
bug1075546

Approval Request Comment
[Feature/regressing bug #]: Bug 619558 (fix in bug 1073577 is incomplete)
[User impact if declined]: Potential security vulnerability
[Describe test coverage new/current, TBPL]: Test case in bug
[Risks and why]: Low
[String/UUID change made/needed]: None

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/75ebb70f8b38

Comment 16

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original issue using the steps from comment #0:

* Build Used: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/10/2014-10-01-mozilla-central-debug/
** Assertion: Assertion failure: entry_ == makeIndex(clasp, key, kind), at /builds/slave/m-cen-osx64-d-0000000000000000/build/js/src/vm/Runtime.h:334

Verified using the following builds:

* Build Used: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-11-21-mozilla-central-debug/
** ran the poc from comment #0 several times (10x) without receiving the original assertion

* Build Used: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-11-21-mozilla-aurora-debug/
** ran the poc from comment #0 several times (10x) without receiving the original assertion

* Build Used: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-11-21-mozilla-beta-debug/
** ran the poc from comment #0 several times (10x) without receiving the original assertion