880697 - JSD often uses JSContexts without pushing

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Sigh. This can lead to compartment mismatches. I'm marking this as sec-moderate because it only really matters when someone is debugging a page with Firebug, and it's not likely that many people will do that to malicious pages.

Comment 1

[Andrew McCreight [:mccr8]]

From Honza, in bug 821733:

Another STR:

1) Install Firebug 1.11.4, https://addons.mozilla.org/en-US/firefox/addon/firebug/
2) Open https://getfirebug.com/tests/1.11/commandLine/4434/issue4434.html
3) Follow the steps -> CRASH

Crash in: js::CompartmentChecker::fail 	js/src/jscntxtinlines.h:165

https://crash-stats.mozilla.com/report/index/bp-d4861756-c34a-4288-b804-b1b322130607

Btw. I believe the crash started in this App Changeset: ddb7b23166ef

Comment 2

[Bobby Holley (:bholley)]

I'm writing a bunch of patches to remove jsdc->dumbContext and just use the SafeJSContext. Not really the way I wanted to spend my morning, but oh well.

Comment 3

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=7287732f7bf5

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add an RAII class to JSD to save/restore exception state. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Stop using clunky C API in JSD and start using RAII classes. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Make JS_ClearScriptTraps take a runtime directly. v1

This obviates the need for a context at the JSD callsite.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 4 - Include XPConnect and grab the SafeJSContext when nsContentUtils has been torn down. v1

We don't really want to be adding xpconnect/src to more LOCAL_INCLUDES, but JSD
is going away, so let's just do this.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 5 - Replace usage of dumbContext with AutoSafeJSContext. v1

dumbContext ends up with jsdc->glob as its default global, so we have to be
very careful to audit for any places where the code might be assuming that
its cx is in the compartment of jsdc->glob. Luckily, the code already seems
pretty explicit about its compartments.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 6 - Remove JSD_GetDefaultJSContext and fix callers. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 7 - Remove dumbContext. v1

Whew.

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 8 - Push in a few other suspicious places. v1

Comment 12

[Bobby Holley (:bholley)]

Looks like my strategy for jsd_DestroyScriptHookProc went orange on opt builds. Terrence is going to write a patch that lets me use a JSRuntime to root. Once he uploads that, I'll modify Part 4 to do that and drop the LOCAL_INCLUDE.

Comment 13

[Terrence Cole [:terrence]]

Attachment: v0: Add constructor for Rooted<>(JSRuntime*)

Comment 14

[Bobby Holley (:bholley)]

Awesome, thanks Terrence!

Comment 15

[Terrence Cole [:terrence]]

Attachment: Part 0 - Add constructor for Rooted<>(JSRuntime*). v1

Actually add the new test.

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 4 - Root directly with a runtime in jsd_DestroyScriptHookProc. v1

Comment 17

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=7c809db43d90

Comment 18

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #0)
> Sigh.

Indeed...

Comment 19

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 759905 [details] [diff] [review]
Part 2 - Stop using clunky C API in JSD and start using RAII classes. v1

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

The dumbContext and cx usage is not very consistent in this patch (sometimes you introduce cx then keep using the dumbContext...) but since you remove the whole thing few patches later I'm just going to ignore it. A few things:

+    JSAutoCompartment ac2(cx, jsdc->glob);
is the 2 really needed here?

in jsd_GetValueString seems like you forgot the auto request

Comment 20

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 760037 [details] [diff] [review]
Part 4 - Root directly with a runtime in jsd_DestroyScriptHookProc. v1

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

Why did the previous version go orange? That sounds scary to me...

Comment 21

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 759908 [details] [diff] [review]
Part 5 - Replace usage of dumbContext with AutoSafeJSContext. v1

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

jsd_NewValue(JSDContext* jsdc, jsval value)
 {
-    JS::RootedValue val(jsdc->dumbContext, value);
-    JSAutoRequest ar(jsdc->dumbContext);
+    AutoSafeJSContext cx;
+    JS::RootedValue val(cx, value);
+    JSAutoRequest ar(cx);

unnecessary JSAutoRequest

 jsd_DropValue(JSDContext* jsdc, JSDValue* jsdval)
 {
     JS_ASSERT(jsdval->nref > 0);
     if(0 == --jsdval->nref)
     {
         jsd_RefreshValue(jsdc, jsdval);
         if(JSVAL_IS_GCTHING(jsdval->val))
         {
-            JSAutoRequest ar(jsdc->dumbContext);
-            JSAutoCompartment ac(jsdc->dumbContext, jsdc->glob);
-            JS_RemoveValueRoot(jsdc->dumbContext, &jsdval->val);
+            AutoSafeJSContext cx;
+            JSAutoRequest ar(cx);

here too

Comment 22

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 759910 [details] [diff] [review]
Part 7 - Remove dumbContext. v1

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

\o/

Comment 23

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 759911 [details] [diff] [review]
Part 8 - Push in a few other suspicious places. v1

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

-    JSContext *cx = JSD_GetJSContext (mCx, mThreadState);
+    AutoPushJSContext cx(JSD_GetJSContext (mCx, mThreadState));
 
     JS::RootedValue jv(cx);
 
     estate = JS_SaveExceptionState (cx);
     JS_ClearPendingException (cx);
 
     nsCxPusher pusher;
     pusher.Push(cx);

the nsCxPusher should now just go

Comment 24

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

Comment on attachment 760032 [details] [diff] [review]
Part 0 - Add constructor for Rooted<>(JSRuntime*). v1

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

We already have rooters in the runtime via mainThread. Can you try re-using those by calling getMainThread or something?

Comment 25

[Terrence Cole [:terrence]]

Attachment: Part 0 - Add constructor For Rooted<>(JSRutnime *). v2

Comment 26

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

Attachment: add JSRuntime ctor for Rooted

Sorry, this is what I meant.

Comment 27

[Terrence Cole [:terrence]]

Comment on attachment 761076 [details] [diff] [review]
add JSRuntime ctor for Rooted

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

r=me

Comment 28

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #19)
> The dumbContext and cx usage is not very consistent in this patch (sometimes
> you introduce cx then keep using the dumbContext...) but since you remove
> the whole thing few patches later I'm just going to ignore it. A few things:
> 
> +    JSAutoCompartment ac2(cx, jsdc->glob);
> is the 2 really needed here?

Technically not. I think avoiding shadowing is always clearer, though.

> in jsd_GetValueString seems like you forgot the auto request

Good catch.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #20)
> Why did the previous version go orange? That sounds scary to me...

Because I was trying to grab the SafeJSContext to use for rooting, but nsContentUtils had already been shut down by that point. The point here is that, with billm's patch, we can just root directly with the runtime, so it's simpler to do that.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #23)
> -    JSContext *cx = JSD_GetJSContext (mCx, mThreadState);
> +    AutoPushJSContext cx(JSD_GetJSContext (mCx, mThreadState));
>  
>      JS::RootedValue jv(cx);
>  
>      estate = JS_SaveExceptionState (cx);
>      JS_ClearPendingException (cx);
>  
>      nsCxPusher pusher;
>      pusher.Push(cx);
> 
> the nsCxPusher should now just go

Agreed - technically it's not entirely equivalent (because the AutoPushJSContext might not push), but this code doesn't seem to be relying on that.

Comment 29

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=a3cf185fe73d

Comment 30

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=19827850b9a8

Comment 31

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/444fffdcf768
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/77b50dfb8b17
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/56c790b9bdcc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d775f43176c8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/58bd67e4294a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c521067ed542
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/30e704196d0a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5f37e9cb1334
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/81d4a3cf9e14

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/444fffdcf768
https://hg.mozilla.org/mozilla-central/rev/77b50dfb8b17
https://hg.mozilla.org/mozilla-central/rev/56c790b9bdcc
https://hg.mozilla.org/mozilla-central/rev/d775f43176c8
https://hg.mozilla.org/mozilla-central/rev/58bd67e4294a
https://hg.mozilla.org/mozilla-central/rev/c521067ed542
https://hg.mozilla.org/mozilla-central/rev/30e704196d0a
https://hg.mozilla.org/mozilla-central/rev/5f37e9cb1334
https://hg.mozilla.org/mozilla-central/rev/81d4a3cf9e14