Closed Bug 908915 Opened 6 years ago Closed 6 years ago

Crash [@ js::CompartmentChecker::fail] or compartment mismatch

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox27 --- fixed
firefox-esr24 27+ fixed

People

(Reporter: gkw, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase, Whiteboard: [fuzzblocker][jsbugmon:])

Attachments

(2 files)

Attached file stack
function f(y) {}
for each(let e in newGlobal()) {
    try {
        e();
    } catch (r) {}
}

crashes js debug (64-bit threadsafe deterministic) shell on m-c changeset d58fc624899c without any CLI arguments at js::CompartmentChecker::fail and shows the following error:

*** Compartment mismatch 0x178ccf0 vs. 0x1773450
Hit MOZ_CRASH()

s-s because compartment mismatches are bad.
This seems to go back for some time, probably past e3799f9cfee8.
Another testcase:

(function() {
    arguments.__proto__.__proto__ = newGlobal()
    function f(y) {
        y()
    }
    for each(b in []) {
        try {
            f(b)
        } catch (e) {}
    }
})()

*** Compartment mismatch 0x29df3c0 vs. 0x29c4460
Hit MOZ_CRASH()
OS: Mac OS X → Linux
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Marking sec-high for compartment mismatch.
Keywords: sec-high
Eric said he might be able to take a peek and see how this can proceed.
Flags: needinfo?(efaustbmo)
OK, so the problem is as follows:

When calling either DecompileThisScript(), or Decompile() with argc 0 through a CCW, we try and decompile the current script having entered the other compartment. When we try and decompile a function we pulled out of that script, we notice.

I have several questions, and I don't even know who best to complain at about all this stuff.

1) Is there some way we can unify these amazingly similar seeming codepaths?
2) Since this is now two different callers of JS_DescribeScriptedCaller(), do we want to effect the changes there, or just mangle these two functions into submission since they are DEBUG only, anyway?

I talked breifly with Jeff about this, and I think I am inclined to agree with his judgement to just fix it in place. Perhaps the easiest way to do that is to do something similar to EvalInContext(), which does:

        Maybe<JSAutoCompartment> ac;
        unsigned flags;
        JSObject *unwrapped = UncheckedUnwrap(sobj, true, &flags);
        if (flags & Wrapper::CROSS_COMPARTMENT) {
            sobj = unwrapped;
            ac.construct(cx, sobj);
        }    

and put it in Disassemble1()?

Jeff, thoughts on this approach?
Flags: needinfo?(efaustbmo) → needinfo?(jwalden+bmo)
Yeah, I think the in-place fix is most reasonable.  Slightly fugly and one-off, but reasonable.

Unification would be good.  On the other hand, if we ever do that AST-parsing thing any unification would be only so much wasted effort, so it's hard for me to get too excited about doing much to improve this.  (Especially as it's basically shell-only functionality, unless the intermediate-expression decompiler -- which I'd thought was totally separate code, but I might be mistaken -- is at all affected by this.)
Flags: needinfo?(jwalden+bmo)
Eric, are you still working on this bug?
Flags: needinfo?(efaustbmo)
Nah, you can take a look at it if you want. It looks like it's debug only, so I didn't prioritize it that highly. The fix is well defined and discussed above.
Flags: needinfo?(efaustbmo)
Can this please be fixed? This prevents us from spotting other compartment mismatch bugs, some of which may be bad and not just be debug-only as per this case.
Flags: needinfo?(nihsanullah)
Whiteboard: [jsbugmon:] → [fuzzblocker][jsbugmon:]
Attached patch PatchSplinter Review
This patch fixes both tests.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Attachment #816598 - Flags: review?(efaustbmo)
Flags: needinfo?(nihsanullah)
Eric, review ping :)
Comment on attachment 816598 [details] [diff] [review]
Patch

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

Yeah, OK. This looks fine. r=me with the test.

I wonder if we can't push it down a little further, but this makes a lot of sense as a place to put the fix.
Attachment #816598 - Flags: review?(efaustbmo) → review+
Shell only so not a sec bug.

https://hg.mozilla.org/integration/mozilla-inbound/rev/315555d51133
Group: core-security
Keywords: sec-high
https://hg.mozilla.org/mozilla-central/rev/315555d51133
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Since this aids fuzzing (by fixing this bug, we may be able to find other bad mismatches), is there any chance this can be nominated for backport?
Flags: needinfo?(jdemooij)
(In reply to Gary Kwong [:gkw] [:nth10sd] (yes, still catching up on bugmail) from comment #16)
> Since this aids fuzzing (by fixing this bug, we may be able to find other
> bad mismatches), is there any chance this can be nominated for backport?

Sorry for the delay. It's shell-only so it should be safe to backport. On which of them should we land this? The patch is on aurora now and will be on beta in < 2 weeks. Are you fuzzing ESR24?
Flags: needinfo?(jdemooij) → needinfo?(gary)
> Sorry for the delay. It's shell-only so it should be safe to backport. On
> which of them should we land this? The patch is on aurora now and will be on
> beta in < 2 weeks. Are you fuzzing ESR24?

I do not fuzz ESR24 not very often, but considering it still has a long lifetime, it will be nice to have it there.

I'm not too worried about this particular bug, but rather that it may mask other compartment mismatches.
Flags: needinfo?(gary)
Comment on attachment 816598 [details] [diff] [review]
Patch

[Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Shell-only problem, helps fuzzers find more serious issues.

> User impact if declined: 
Makes fuzzing harder.

> Fix Landed on Version:
27

> Risk to taking this patch (and alternatives if risky): 
JS-shell only, shouldn't affect the browser build at all.

> String or UUID changes made by this patch: 
None.
Attachment #816598 - Flags: approval-mozilla-esr24?
We'll take this fix alongside Firefox 27, just to get some confidence in the fix prior to release.
Alex, this patch has been waiting for approval for over a month. It's shell-only, so it doesn't affect the browser at all.
Flags: needinfo?(akeybl)
FWIW, it's better to put ni? etc. on the RelMan team and not at a single member of it.
Flags: needinfo?(akeybl) → needinfo?(release-mgmt)
Attachment #816598 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
Flags: needinfo?(release-mgmt)
You need to log in before you can comment on or make changes to this bug.