Closed Bug 780712 Opened 11 years ago Closed 11 years ago

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


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox14 --- wontfix
firefox15 --- fixed
firefox16 --- fixed
firefox17 --- fixed
firefox-esr10 --- unaffected


(Reporter: gkw, Assigned: luke)



(4 keywords, Whiteboard: [advisory-tracking+][qa?])

Crash Data


(2 files)

Attached file stacks
r = evalcx("/x/", undefined);
s = "";

crashes js debug shell on m-c changeset 6c60e99d9739 without any CLI arguments at JSC::Yarr::execute with js::RegExpShared::execute on the stack.
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.
Assignee: general → wmccloskey
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:

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

    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.
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.
Attached patch fix and testSplinter Review
Attachment #649678 - Flags: review?(wmccloskey)
Comment on attachment 649678 [details] [diff] [review]
fix and test

Thanks. I hadn't realized that CrossCompartmentWrapper overrode all these.
Attachment #649678 - Flags: review?(wmccloskey) → review+
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
Attachment #649678 - Flags: approval-mozilla-beta?
Attachment #649678 - Flags: approval-mozilla-aurora?
This will probably be WONTFIX for Firefox 14.
Assignee: wmccloskey → luke
Closed: 11 years ago
Resolution: --- → FIXED
Crash Signature: [@ JSC::Yarr::execute] [@ js::RegExpShared::execute] → [@ JSC::Yarr::execute] [@ js::RegExpShared::execute]
JSBugMon: This bug has been automatically verified fixed.
Attachment #649678 - Flags: approval-mozilla-beta?
Attachment #649678 - Flags: approval-mozilla-beta+
Attachment #649678 - Flags: approval-mozilla-aurora?
Attachment #649678 - Flags: approval-mozilla-aurora+
Crash Signature: [@ JSC::Yarr::execute] [@ js::RegExpShared::execute] → [@ JSC::Yarr::execute] [@ js::RegExpShared::execute]
Keywords: verifyme
Whiteboard: [advisory-tracking+]
Does this need verification given comment 10?
Keywords: verifyme
Whiteboard: [advisory-tracking+] → [advisory-tracking+][qa?]
Group: core-security
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/testBug780712.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.