780712 - Crash [@ JSC::Yarr::execute] or [@ js::RegExpShared::execute]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stacks

r = evalcx("/x/", undefined);
s = "";
gc()
Function("\
    s.match(r);\
    schedulegc(__proto__);\
    ({c:schedulegc(2)});\
    s.match(r);\
")()

crashes js debug shell on m-c changeset 6c60e99d9739 without any CLI arguments at JSC::Yarr::execute with js::RegExpShared::execute on the stack.

Comment 1

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   91130:15a23c3923ff
user:        Bill McCloskey
date:        Tue Apr 03 14:07:38 2012 -0700
summary:     Bug 742570 - Improve GC testing functions (r=igor)

Not sure if this only affects gc testing functions, so setting Firefox 14 and later as affected.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Here's what seems to be happening. We have a cross-compartment wrapper to a regexp. We call some regexp method on it, and that calls RegExpToShared on the CCW. That calls through some proxy code, which basically unwraps the regexp and calls RegExpToShared on the unwrapped regexp. We don't have a RegExpShared for this regexp yet, so we call RegExpObject::createShared, which looks like this:

bool
RegExpObject::createShared(JSContext *cx, RegExpGuard *g)
{
    Rooted<RegExpObject*> self(cx, this);

    JS_ASSERT(!maybeShared());
    if (!cx->compartment->regExps.get(cx, getSource(), getFlags(), g))
        return false;

    self->setShared(cx, **g);
    return true;
}

The crucial thing here is that while self is the unwrapped regexp, cx->compartment is the compartment that the CCW was in. So they don't match. Consequently, we're putting the RegExpShared in the wrong compartment's table. Later on, we collect that compartment and kill the RegExpShared, but the |self| object from above still has a pointer to it because it was in a different compartment and not traced.

An easy fix would be to change cx->compartment to self->compartment(). However, it seems like this is a more general problem. I see a number of IndirectProxyHandler methods that unwrap a CCW and do some work on the unwrapped object without ever switching compartments. This seems pretty dangerous to me. Is it by design? Can we change it so that we enter the right compartment? We'd have to wrap the result, but that doesn't seem to be an issue in these cases as far as I can tell.

Comment 3

[Luke Wagner [:luke]]

Sorry you had to look into this.  It seems to me like the fix would be for CrossCompartmentWrapper to override regexp_toShared, wrapping the call to IndirectProxyHandler::regexp_toShared with an AutoCompartment, as with most of the rest of the ProxyHandler hooks.

Comment 4

[Luke Wagner [:luke]]

Attachment: fix and test

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 649678 [details] [diff] [review]
fix and test

Thanks. I hadn't realized that CrossCompartmentWrapper overrode all these.

Comment 6

[Luke Wagner [:luke]]

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

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 649678 [details] [diff] [review]
fix and test

[Approval Request Comment]
User impact if declined: memory corruption
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low

Comment 8

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

This will probably be WONTFIX for Firefox 14.

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a2398a7be210

Comment 10

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 11

[Luke Wagner [:luke]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6372bb9fb95d
https://hg.mozilla.org/releases/mozilla-beta/rev/ac9af7f6b0d1

Comment 12

[u279076]

Does this need verification given comment 10?

Comment 13

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/testBug780712.js.