"Assertion failure: thing->compartment() == rt->gcCheckCompartment || thing->compartment() == rt->atomsCompartment,"

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: gkw, Assigned: billm)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla10
x86
Mac OS X
assertion, testcase
Points:
---

Firefox Tracking Flags

(firefox6- wontfix, firefox7- wontfix, firefox8+ affected, firefox9+ affected)

Details

(Whiteboard: js-triage-done [qa-])

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
Created attachment 558546 [details]
stack

(evalcx(''))[''] = Proxy.create((function(x) {
  return {
    r: function() {
      for (e in x) {}
    }
  }
})(new(function() {
  yield
})("", /x/)), (b = 6))
gc(function() {})


asserts js debug shell on m-c changeset db9e99d537f2 with the patch from bug 683966 with -m and -a at Assertion failure: thing->compartment() == rt->gcCheckCompartment || thing->compartment() == rt->atomsCompartment,

Even though this concerns evalcx, gc seems to be involved so locking s-s just to be safe.
Whiteboard: js-triage-needed → js-triage-needed, [sg:critical?]
What platform is this?  I can't repro.
(Reporter)

Comment 2

6 years ago
(In reply to Brian Hackett from comment #1)
> What platform is this?  I can't repro.

Mac OS X 10.6.
status-firefox7: --- → wontfix
status-firefox8: --- → affected
status-firefox9: --- → affected
tracking-firefox7: --- → -

Updated

6 years ago
status-firefox6: --- → wontfix
tracking-firefox6: --- → -
tracking-firefox8: --- → +
tracking-firefox9: --- → +
I think this one might be a bogus assert. We are asserting on a compartment check during GC mark. The object being marked is a sandbox, the compartment of which does not match the GC compartment. We get to that object through generator_trace, via this line of code:

    /*
     * Currently, generators are not mjitted. Still, (overflow) args can be
     * pushed by the mjit and need to be conservatively marked. Technically, the
     * formal args and generator slots are safe for exact marking, but since the
     * plan is to eventually mjit generators, it makes sense to future-proof
     * this code and save someone an hour later.
     */
    MarkStackRangeConservatively(trc, gen->floatingStack, fp->formalArgsEnd());

I suspect we are conservatively marking something we don't have to mark, and getting to the sandbox that way.

Bill, can you check this out?
Wouldn't the bug be in the sandbox marking itself, then?  I don't see how conservatively marking any address range could itself be a correctness bug, since any word the conservative GC finds could also be some random integer value found in the area it needs to be looking at.
(Assignee)

Comment 5

6 years ago
Created attachment 562510 [details] [diff] [review]
patch

Dave is right; this assertion is overzealous. Normally a per-compartment GC will never mark outside its compartment once drainMarkStack has begun. But since the generator code does conservative scanning, this invariant can be broken. The patch just skips the assertion during the conservative phase.

I'm not exactly sure how the sandbox pointer ends up on the JS stack. But it only happens with overflow arguments when we're in the mjit code. So I'm going to trust the comment in generator_trace that this is a valid thing to have happen.
Assignee: general → wmccloskey
Status: NEW → ASSIGNED
Attachment #562510 - Flags: review?(luke)
Group: core-security
Whiteboard: js-triage-needed, [sg:critical?] → js-triage-done

Comment 6

6 years ago
Comment on attachment 562510 [details] [diff] [review]
patch

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

whodathunkit
Attachment #562510 - Flags: review?(luke) → review+
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e872ae00cdc
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/5e872ae00cdc
https://hg.mozilla.org/mozilla-central/rev/a0eade7fa486
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
qa- as nothing to do for QA fix verification -- please set to qa+ if this is not the case.
Whiteboard: js-triage-done → js-triage-done [qa-]

Comment 10

6 years ago
Do we need this ported to Fx9? Looks like we shipped with this in Fx8 to no (major) ill effects...
(Assignee)

Comment 11

6 years ago
This is a bad assertion, so it won't have any effect on release builds.
You need to log in before you can comment on or make changes to this bug.