942530 - Assertion failure: obj->compartment() == obj2->compartment(), at c:\work\mozilla\builds\nightly\mozilla\js\src\gc/Marking.cpp:1448

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Carsten Book [:Tomcat]]

same issue as bug 942529 but someone different assertion on a Windows 7 Debug Build, loading a local copy of https://codenvy.com/ide/edunogodz caused Assertion failure: obj->compartment() == obj2->compartment(), at c:\work\mozilla\builds\nightly\mozilla\js\src\gc/Marking.cpp:1448

will try to generate a testcase

Comment 1

[Carsten Book [:Tomcat]]

Attachment: windows stack

Comment 2

[Carsten Book [:Tomcat]]

working on a local testcase currently

Comment 3

[Andrew McCreight [:mccr8]]

Sounds like a compartment mismatch.

Comment 4

[Jan de Mooij [:jandem]]

Tomcat, can you still reproduce this?

Comment 5

[Carsten Book [:Tomcat]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> Tomcat, can you still reproduce this?

well, i can still reproduce the crash loading the site https://codenvy.com/ide/edunogodz  in a super new tip debug build on win7 :) but the issue is, that the assertion has now changed its now 

Assertion failure: GetGCThingTraceKind(rope) == JSTRACE_STRING, at c:\Users\mozilla\debug-builds\mozilla-central\js\src\gc/Marking.cpp:961

Comment 6

[Carsten Book [:Tomcat]]

(In reply to Carsten Book [:Tomcat] from comment #5)
> (In reply to Jan de Mooij [:jandem] from comment #4)
> > Tomcat, can you still reproduce this?
> 
> well, i can still reproduce the crash loading the site
> https://codenvy.com/ide/edunogodz  in a super new tip debug build on win7 :)
> but the issue is, that the assertion has now changed its now 
> 
> Assertion failure: GetGCThingTraceKind(rope) == JSTRACE_STRING, at
> c:\Users\mozilla\debug-builds\mozilla-central\js\src\gc/Marking.cpp:961

erm ok was reading my own bug wrong :) Loading the Site from the web result in the assertion from comment 4 -> bug 942529  but loading the same site after grabbing via wget -> the assertion from comment #0

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Carsten Book [:Tomcat] from comment #6)
> erm ok was reading my own bug wrong :) Loading the Site from the web result
> in the assertion from comment 4 -> bug 942529  but loading the same site
> after grabbing via wget -> the assertion from comment #0

OK, it's likely the same underlying problem though. It would be great to have a testcase; both asserts are scary.

Comment 9

[Carsten Book [:Tomcat]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> OK, it's likely the same underlying problem though. It would be great to
> have a testcase; both asserts are scary.

Hm i tried about a week to reduce the testcase with lithium and in the end the reduced testcase didn't crashed anymore (and still don't crash when i load the page now again), so seems this GC crashes are really hard to catch.

Comment 10

[Jan de Mooij [:jandem]]

I can reproduce this with a Windows debug build I downloaded from tbpl. Unfortunately it doesn't crash on OS X.

My Windows debugging skills aren't very good, but I will see if I can get some info before they modify their website.

Comment 11

[Jan de Mooij [:jandem]]

Looks like we have some bogus string/object values in the GC heap. There may be a rogue SETELEM somewhere: I can't reproduce this if I disable Ion's dense SETELEM path, but that could just be hiding the problem.

As a first step, I want to add a new debugging mode for Ion code where we check that every string/object/Value that flows between MIR instructions is "sane": valid arena, valid zone/compartment, matches known type info etc. It may or may not help here, but it shouldn't be that much work and could also be really useful for fuzzing.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: patch.txt

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Patch

With the extra type checks I just landed for bug 949475, I was able to track this down. We had an MLoadSlot (JSOP_GETGNAME) that was loading a function instead of an array, and the TypeObject check failed.

The problem is this enum in jsinfer.h:

enum {
    ...
    TYPE_FLAG_DEFINITE_MASK       = 0xfffe0000,
    TYPE_FLAG_DEFINITE_SHIFT      = 17

MSVC uses signed enums and miscompiles this, and we end up with totally bogus definite slots if the global has more than 32767 (!) slots.

This patch uses MOZ_ENUM_TYPE, but that alone didn't fix it, even though it should be defined for MSVC 2010. I also had to add a cast to canSetDefinite.

Regression from bug 930048 / bug 932769 (Firefox 28).

Comment 14

[Jan de Mooij [:jandem]]

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

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

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No, there's a comment but it doesn't add that much info compared to the rest of the patch.

> Which older supported branches are affected by this flaw?
28+

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

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

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

Comment 15

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

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

Sec-approval+ for trunk and I approved it for Aurora after that.

Comment 16

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e65d37d2e3e4

Comment 17

[Carsten Book [:Tomcat]]

landed on central - super great thanks jan!

-> https://hg.mozilla.org/mozilla-central/rev/e65d37d2e3e4

setting in testsuite just in case we are to have a handy test

Comment 18

[Jan de Mooij [:jandem]]

(In reply to Carsten Book [:Tomcat] from comment #17)
> landed on central - super great thanks jan!

Thank you for reporting this; I'm glad we fixed this one before it hit a release :)

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3b3c685fdc33

Comment 20

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed assert in Fx28 2013-12-13.
Verified fixed in Fx28 and Fx29, 2013-12-20.