Closed Bug 951366 Opened 7 years ago Closed 7 years ago

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


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox26 --- wontfix
firefox27 + fixed
firefox28 + fixed
firefox29 + fixed
firefox-esr24 27+ fixed
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed


(Reporter: decoder, Assigned: jandem)


(Keywords: assertion, sec-critical, testcase, Whiteboard: [jsbugmon:update][adv-main27+][adv-esr24.3+])


(2 files)

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

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);
Needinfo from bhackett based on gcPreserveCode() and bug 932982 landing recently.
Flags: needinfo?(bhackett1024)
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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?
Flags: needinfo?(bhackett1024) → needinfo?(jdemooij)
Attached patch PatchSplinter Review
(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.
Assignee: nobody → jdemooij
Attachment #8349538 - Flags: review?(bhackett1024)
Flags: needinfo?(jdemooij)
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.
Keywords: sec-critical
Comment on attachment 8349538 [details] [diff] [review]

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.
Attachment #8349538 - Flags: review?(bhackett1024) → review?(hv1989)
Comment on attachment 8349538 [details] [diff] [review]

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.
Attachment #8349538 - Flags: review?(hv1989) → review+
Comment on attachment 8349538 [details] [diff] [review]

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.): 

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

> String or IDL/UUID changes made by this patch:
Attachment #8349538 - Flags: sec-approval?
Attachment #8349538 - Flags: approval-mozilla-esr24?
Attachment #8349538 - Flags: approval-mozilla-beta?
Attachment #8349538 - Flags: approval-mozilla-aurora?
Comment on attachment 8349538 [details] [diff] [review]

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.
Attachment #8349538 - Flags: sec-approval?
Attachment #8349538 - Flags: sec-approval+
Attachment #8349538 - Flags: approval-mozilla-beta?
Attachment #8349538 - Flags: approval-mozilla-beta+
Attachment #8349538 - Flags: approval-mozilla-aurora?
Attachment #8349538 - Flags: approval-mozilla-aurora+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
JSBugMon: This bug has been automatically verified fixed.
Flags: needinfo?(bbajaj)
Attachment #8349538 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main27+][adv-esr24.3+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.