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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This seems to go back for some time, probably past e3799f9cfee8.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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()

Comment 3

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 4

[Andrew McCreight [:mccr8]]

Marking sec-high for compartment mismatch.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Eric said he might be able to take a peek and see how this can proceed.

Comment 6

[Eric Faust [:efaust]]

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?

Comment 7

[Jeff Walden [:Waldo]]

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

Comment 8

[Nicolas B. Pierron [:nbp]]

Eric, are you still working on this bug?

Comment 9

[Eric Faust [:efaust]]

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.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Patch

This patch fixes both tests.

Comment 12

[Jan de Mooij [:jandem]]

Eric, review ping :)

Comment 13

[Eric Faust [:efaust]]

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.

Comment 14

[Jan de Mooij [:jandem]]

Shell only so not a sec bug.

https://hg.mozilla.org/integration/mozilla-inbound/rev/315555d51133

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/315555d51133

Comment 16

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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?

Comment 17

[Jan de Mooij [:jandem]]

(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?

Comment 18

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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

Comment 19

[Jan de Mooij [:jandem]]

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.

Comment 20

[Alex Keybl [:akeybl]]

We'll take this fix alongside Firefox 27, just to get some confidence in the fix prior to release.

Comment 21

[Jan de Mooij [:jandem]]

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.

Comment 22

[Robert Kaiser]

FWIW, it's better to put ni? etc. on the RelMan team and not at a single member of it.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/5e401488703e