Closed Bug 1233152 Opened 8 years ago Closed 8 years ago

Crash [@ js::CompartmentChecker::fail] or Crash [@ compartment]

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla46
Tracking Status
firefox43 --- wontfix
firefox44 + fixed
firefox45 + verified
firefox46 + verified
firefox-esr38 44+ fixed
firefox-esr45 --- fixed
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- wontfix
b2g-v2.1S --- wontfix
b2g-v2.2 --- affected
b2g-v2.5 --- affected
b2g-v2.2r --- affected
b2g-master --- fixed

People

(Reporter: decoder, Assigned: jandem)

Details

(4 keywords, Whiteboard: [jsbugmon:update][adv-main44+][adv-esr38.6+])

Crash Data

Attachments

(1 file)

The following testcase crashes on mozilla-central revision 749f9328dd76 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2):

gczeal(2, 10);
var g = newGlobal();
var elt = new g.Object;
g.offThreadCompileScript('debugger;', {
    element: elt,
});
var g = newGlobal();
g.runOffThreadScript();


Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x00000000005c7ea0 in js::CompartmentChecker::fail (c1=<optimized out>, c2=<optimized out>) at js/src/jscntxtinlines.h:49
#0  0x00000000005c7ea0 in js::CompartmentChecker::fail (c1=<optimized out>, c2=<optimized out>) at js/src/jscntxtinlines.h:49
#1  0x00000000005c7ed5 in js::CompartmentChecker::check (this=<optimized out>, c=<optimized out>) at js/src/jscntxtinlines.h:70
#2  0x0000000000a19be7 in check (script=<optimized out>, this=0x7fffffffc740) at js/src/jscntxtinlines.h:130
#3  check<JSScript*> (rooted=..., this=0x7fffffffc740) at js/src/jscntxtinlines.h:86
#4  assertSameCompartment<JS::Rooted<JSScript*> > (t1=..., cx=0x7ffff6907400) at js/src/jscntxtinlines.h:162
#5  js::GlobalHelperThreadState::finishParseTask (this=<optimized out>, maybecx=maybecx@entry=0x7ffff6907400, rt=rt@entry=0x7ffff695d000, token=token@entry=0x7ffff46d76f0) at js/src/vm/HelperThreads.cpp:1052
#6  0x00000000008a895e in JS::FinishOffThreadScript (maybecx=maybecx@entry=0x7ffff6907400, rt=rt@entry=0x7ffff695d000, token=token@entry=0x7ffff46d76f0) at js/src/jsapi.cpp:4181
#7  0x0000000000487d43 in runOffThreadScript (cx=0x7ffff6907400, argc=<optimized out>, vp=0x7fffffffca48) at js/src/shell/js.cpp:3668
#8  0x0000000000a7d572 in js::CallJSNative (cx=0x7ffff6907400, native=0x487c90 <runOffThreadScript(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
#9  0x0000000000a75b27 in js::Invoke (cx=cx@entry=0x7ffff6907400, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:444
#10 0x0000000000a767b9 in js::Invoke (cx=cx@entry=0x7ffff6907400, thisv=..., fval=..., argc=<optimized out>, argv=0x7ffff46710b0, rval=...) at js/src/vm/Interpreter.cpp:496
#11 0x000000000099c2e7 in js::DirectProxyHandler::call (this=this@entry=0x1babf10 <js::CrossCompartmentWrapper::singleton>, cx=cx@entry=0x7ffff6907400, proxy=..., proxy@entry=..., args=...) at js/src/proxy/DirectProxyHandler.cpp:77
#12 0x00000000009a3ab2 in js::CrossCompartmentWrapper::call (this=0x1babf10 <js::CrossCompartmentWrapper::singleton>, cx=0x7ffff6907400, wrapper=..., args=...) at js/src/proxy/CrossCompartmentWrapper.cpp:289
#13 0x00000000009a0852 in js::Proxy::call (cx=0x7ffff6907400, proxy=proxy@entry=..., args=...) at js/src/proxy/Proxy.cpp:391
#14 0x00000000009a0922 in js::proxy_Call (cx=<optimized out>, argc=<optimized out>, vp=<optimized out>) at js/src/proxy/Proxy.cpp:683
#15 0x0000000000a7d572 in js::CallJSNative (cx=0x7ffff6907400, native=0x9a0870 <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
[...]
#27 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:6877
rax	0x0	0
rbx	0x7ffff6907400	140737330050048
rcx	0x7ffff6ca53cd	140737333842893
rdx	0x0	0
rsi	0x7ffff6f7a9d0	140737336814032
rdi	0x7ffff6f791c0	140737336807872
rbp	0x7fffffffc6f0	140737488340720
rsp	0x7fffffffc6f0	140737488340720
r8	0x7ffff7fe0780	140737354008448
r9	0x6372732f736a2f6c	7165916604736876396
r10	0x7fffffffc4b0	140737488340144
r11	0x7ffff6c27960	140737333328224
r12	0x7fffffffc740	140737488340800
r13	0x7fffffffc760	140737488340832
r14	0x7fffffffc780	140737488340864
r15	0x7ffff6907408	140737330050056
rip	0x5c7ea0 <js::CompartmentChecker::fail(JSCompartment*, JSCompartment*)+48>
=> 0x5c7ea0 <js::CompartmentChecker::fail(JSCompartment*, JSCompartment*)+48>:	movl   $0x31,0x0
   0x5c7eab <js::CompartmentChecker::fail(JSCompartment*, JSCompartment*)+59>:	callq  0x4a3db0 <abort()>


This leads to a use-after-free according to the debug error message. However, it doesn't seem to be s-s because it requires the Debugger.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/1415320cf150
user:        Jeff Walden
date:        Thu Sep 24 12:51:56 2015 -0700
summary:     Bug 1101561 - Fix generator bootstrapping (for legacy and star generators both) to be OOM-safe.  r=jandem

This iteration took 263.832 seconds to run.
The bisection window in comment 1 isn't accurate. The following one is probably more relevant:

=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20151209185239" and the hash "6fce35b1c9877805e071baa91136f372091ccc66".
The "bad" changeset has the timestamp "20151209195945" and the hash "151ce2b0e3f6b73505be35561f148678577dcbcb".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=6fce35b1c9877805e071baa91136f372091ccc66&tochange=151ce2b0e3f6b73505be35561f148678577dcbcb

which points to bug 1225396. Jan, is bug 1225396 a likely regressor?
Blocks: 1225396
Flags: needinfo?(jdemooij)
More specifically, this patch:

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/de72e2291ae8
user:        Jan de Mooij
date:        Wed Dec 09 22:55:50 2015 -0500
summary:     Bug 1225396 part 3 - Make %GeneratorPrototype% inherit from %IteratorPrototype%. r=jorendorff
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #2)
> Jan, is bug 1225396 a likely regressor?

No, this is an unrelated and older issue. In GlobalHelperThreadState::finishParseTask, we do this:

    if (!parseTask->finish(cx))
        return nullptr;

    RootedScript script(rt, parseTask->script);
    assertSameCompartment(cx, script);

The finish() call can GC, but ParseTask::script is a plain JSScript* so it can be collected...

I think ParseTask::script should just be a PersistentRootedScript or something.
No longer blocks: 1225396
Group: javascript-core-security
Attached patch PatchSplinter Review
This uses PersistentRooted* for ParseTask::script and ParseTask::sourceObject.

ParseTask has other PersistentRooted members so this should be fine.

Later I'll check which branches are affected.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8699485 - Flags: review?(terrence)
Should the hazard analysis have caught this? I guess not because the ParseTask is not allocated on the stack...
Keywords: sec-high
Comment on attachment 8699485 [details] [diff] [review]
Patch

Review of attachment 8699485 [details] [diff] [review]:
-----------------------------------------------------------------

Ouch!
Attachment #8699485 - Flags: review?(terrence) → review+
(In reply to Jan de Mooij [:jandem] from comment #6)
> Should the hazard analysis have caught this? I guess not because the
> ParseTask is not allocated on the stack...

That is correct. It assumes correct tracing.
Comment on attachment 8699485 [details] [diff] [review]
Patch

[Security approval request comment]
* How easily could an exploit be constructed based on the patch?
It's hard and maybe not exploitable at all. You have to trigger an off-thread parse, then provide it with an 'element' from another compartment (I don't know if this is even possible in the browser..) so we call wrap(), and then you also have to trigger a major GC at that point. Let's assume it's possible though.

* Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

* Which older supported branches are affected by this flaw?
All. I think this regressed in bug 1031636, not sure.

* Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be easy.

* How likely is this patch to cause regressions; how much testing does it need?
Low risk; it just roots some pointers.
Attachment #8699485 - Flags: sec-approval?
Comment on attachment 8699485 [details] [diff] [review]
Patch

sec-approval+ for trunk.

We'll want Aurora, Beta, and ESR38 patches made and nominated once it is checked into trunk.
Attachment #8699485 - Flags: sec-approval? → sec-approval+
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Jan, could you please nominate patch for uplift to Beta, Aurora, esr38? Thanks!
Flags: needinfo?(jdemooij)
Comment on attachment 8699485 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Likely a problem since bug 1031636.
[User impact if declined]: Potential crashes or security issues.
[Describe test coverage new/current, TreeHerder]: Fixes the reported test case.
[Risks and why]: Low risk. It roots some class members in a way that's similar to what we do for other class members.
[String/UUID change made/needed]: None.
Flags: needinfo?(jdemooij)
Attachment #8699485 - Flags: approval-mozilla-esr38?
Attachment #8699485 - Flags: approval-mozilla-beta?
Attachment #8699485 - Flags: approval-mozilla-aurora?
Attachment #8699485 - Flags: approval-mozilla-esr38?
Attachment #8699485 - Flags: approval-mozilla-esr38+
Attachment #8699485 - Flags: approval-mozilla-beta?
Attachment #8699485 - Flags: approval-mozilla-beta+
Attachment #8699485 - Flags: approval-mozilla-aurora?
Attachment #8699485 - Flags: approval-mozilla-aurora+
This doesn't apply cleanly to esr38. Can we get a rebased patch if this needs to go there?
Flags: needinfo?(jdemooij)
Group: javascript-core-security → core-security-release
JSBugMon: This bug has been automatically verified fixed on Fx45
Decoder, would it be easy to verify this fix on Fx44 also? If not, just say so. Thanks.
Flags: needinfo?(choller)
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main44+][adv-esr38.6+]
(In reply to Matt Wobensmith [:mwobensmith][:matt] from comment #21)
> Decoder, would it be easy to verify this fix on Fx44 also? If not, just say
> so. Thanks.

Sorry, I didn't see that request earlier. Was 44 already release at that time? JSBugMon doesn't verify the release branch but only aurora/beta. We could make it verify release but I guess it's rarely ever needed?
Flags: needinfo?(choller)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.