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

VERIFIED FIXED in Firefox 15

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: gkw, Assigned: luke)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla17
x86
Linux
crash, regression, sec-critical, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox14 wontfix, firefox15 fixed, firefox16 fixed, firefox17 fixed, firefox-esr10 unaffected)

Details

(Whiteboard: [advisory-tracking+][qa?], crash signature)

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Created attachment 649406 [details]
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.
(Reporter)

Comment 1

5 years ago
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.
Blocks: 742570
status-firefox-esr10: --- → unaffected
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → 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:

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

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
Created attachment 649678 [details] [diff] [review]
fix and test
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+
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2398a7be210
Target Milestone: --- → mozilla17
(Assignee)

Comment 7

5 years ago
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?
(Reporter)

Comment 8

5 years ago
This will probably be WONTFIX for Firefox 14.
status-firefox14: affected → wontfix
Assignee: wmccloskey → luke

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/a2398a7be210
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox17: affected → fixed
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
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+
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/6372bb9fb95d
https://hg.mozilla.org/releases/mozilla-beta/rev/ac9af7f6b0d1
Crash Signature: [@ JSC::Yarr::execute] [@ js::RegExpShared::execute] → [@ JSC::Yarr::execute] [@ js::RegExpShared::execute]
status-firefox15: affected → fixed
status-firefox16: affected → fixed
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.