Last Comment Bug 780712 - Crash [@ JSC::Yarr::execute] or [@ js::RegExpShared::execute]
: Crash [@ JSC::Yarr::execute] or [@ js::RegExpShared::execute]
: crash, regression, sec-critical, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
-- critical (vote)
: mozilla17
Assigned To: Luke Wagner [:luke]
: general
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: jsfunfuzz 742570
  Show dependency treegraph
Reported: 2012-08-06 13:54 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2013-01-14 08:24 PST (History)
8 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

stacks (8.11 KB, text/plain)
2012-08-06 13:54 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
fix and test (2.49 KB, patch)
2012-08-07 09:52 PDT, Luke Wagner [:luke]
wmccloskey: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image Gary Kwong [:gkw] [:nth10sd] 2012-08-06 13:54:22 PDT
Created attachment 649406 [details]

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.
Comment 1 User image Gary Kwong [:gkw] [:nth10sd] 2012-08-06 14:12:10 PDT
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 User image Bill McCloskey (:billm) 2012-08-06 17:35:07 PDT
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.
Comment 3 User image Luke Wagner [:luke] 2012-08-06 18:28:28 PDT
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 User image Luke Wagner [:luke] 2012-08-07 09:52:36 PDT
Created attachment 649678 [details] [diff] [review]
fix and test
Comment 5 User image Bill McCloskey (:billm) 2012-08-07 10:27:56 PDT
Comment on attachment 649678 [details] [diff] [review]
fix and test

Thanks. I hadn't realized that CrossCompartmentWrapper overrode all these.
Comment 7 User image Luke Wagner [:luke] 2012-08-07 10:45:49 PDT
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 User image Gary Kwong [:gkw] [:nth10sd] 2012-08-07 10:57:50 PDT
This will probably be WONTFIX for Firefox 14.
Comment 9 User image Ed Morley [:emorley] 2012-08-08 10:36:00 PDT
Comment 10 User image Christian Holler (:decoder) 2012-08-08 11:37:52 PDT
JSBugMon: This bug has been automatically verified fixed.
Comment 12 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-16 15:01:07 PDT
Does this need verification given comment 10?
Comment 13 User image Christian Holler (:decoder) 2013-01-14 08:24:23 PST
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/testBug780712.js.

Note You need to log in before you can comment on or make changes to this bug.