951366 - Assertion failure: obj, at dist/include/js/Value.h:527 or Crash on Heap with gcPreserveCode

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision b980c2dee2e7 (run with --fuzzing-safe --ion-eager):


gcPreserveCode();
function assertEq(setter) {}
function raisesException(exception) {
    try {    } catch (actual) {  };
    if (typeof a == 'object') {
        for (var prop in a) {        }
    }
}
function build_getter(i) {
    var x = [ 1 ] ;
    return function f() { return x; }
}
function test() {
    var N = internalConst("INCREMENTAL_MARK_STACK_BASE_CAPACITY") + 2;
    var o = {};
    var descriptor = { enumerable: true};
    for (var i = (0); i != N; ++i) {
	descriptor.get = build_getter(i);
	Object.defineProperty(o, i, descriptor);
    }
    for (var i = 0; i != raisesException; ++i)
	assertEq(o[i][0], i);
}
evaluate("test();");

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Jan de Mooij [:jandem]]

Needinfo from bhackett based on gcPreserveCode() and bug 932982 landing recently.

Comment 3

[Brian Hackett [Laid off!]]

The o[i] access eventually hits the end of the array and returns an undefined value.  At this point the IonScript is correctly invalidated but somehow we end up trying to decode the safepoint after the o[i] which treats the result of the o[i] as if it is still an object.  Since the IonScript is marked for recompilation at the right point this seems to be a malfunction in Ion's invalidation mechanism; I have no real idea how this works and no real interest in learning so can you look at this Jan?

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

(In reply to Brian Hackett (:bhackett) from comment #3)
> Since the IonScript is
> marked for recompilation at the right point this seems to be a malfunction
> in Ion's invalidation mechanism; I have no real idea how this works and no
> real interest in learning so can you look at this Jan?

In this case the AutoDetectInvalidation mechanism is supposed to store the Value in the runtime, and the bailout code uses this to override the bogus return value.

However, GetElementIC::update didn't use AutoDetectInvalidation if the cache was diabled. I'm surprised we didn't find this earlier, it's pretty bad.

Comment 5

[Jan de Mooij [:jandem]]

Looking at the code, this goes all the way back to bug 807498, but I'm not sure why the fuzzers hit this only after bug 932982 landed. Since the patch is very simple, we should just backport this.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8349538 [details] [diff] [review]
Patch

Switching review to Hannes so that we can hopefully land this soon to help the fuzzers, in case Brian is on PTO already. Patch just moves two lines.

Comment 7

[Hannes Verschore [:h4writer]]

Comment on attachment 8349538 [details] [diff] [review]
Patch

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

It is indeed strange we only hit this with OMTC. Would be interesting to know! Nevertheless this looks like a bug that needs to get squashed.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8349538 [details] [diff] [review]
Patch

The fuzzers found this bug after bug 932982 landed. The bug has been here since bug 807498 (Firefox 21) but the fuzzers didn't find it sooner, so it was probably much harder to trigger without bug 932982. But it may be possible somehow, so I think we should backport this just to be safe (and it's a very simple patch).

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

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Patch here has a testcase but I'll land the patch without it.

> Which older supported branches are affected by this flaw?
Firefox 21+.

> If not all supported branches, which bug introduced the flaw?
Bug 807498.

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

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

[Approval Request Comment]
> Bug caused by (feature/regressing bug #):
Bug 807498.

> User impact if declined: 
Crashes, sec bugs, broken websites.

> Testing completed (on m-c, etc.): 
None.

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

> String or IDL/UUID changes made by this patch:
None.

Comment 9

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

Comment on attachment 8349538 [details] [diff] [review]
Patch

sec-approval+ on trunk for landing *without* the test. We don't land security tests until six weeks after we ship the fix public normally as it can give away the problem.

I've also approved it for Aurora and Beta without the test. We need Release Management to say ok on ESR24.

Comment 10

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9189cf0e947f

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9189cf0e947f

Comment 12

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/af875089faa5
https://hg.mozilla.org/releases/mozilla-beta/rev/4fbd55011787

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/b5cacefa3af6
https://hg.mozilla.org/releases/mozilla-esr24/rev/d9ab941d3624

We still need to get this approved for esr24.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Epic rebasing fail on my part. These actually build.

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/a76212512e66
https://hg.mozilla.org/releases/mozilla-esr24/rev/b558ccae70b7