Last Comment Bug 880697 - JSD often uses JSContexts without pushing
: JSD often uses JSContexts without pushing
Status: RESOLVED FIXED
[adv-main24-]
: sec-moderate
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla24
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
:
Mentors:
Depends on:
Blocks: compartment-mismatch
  Show dependency treegraph
 
Reported: 2013-06-07 08:27 PDT by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2014-11-19 20:07 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
wontfix
unaffected
wontfix
wontfix
unaffected


Attachments
Part 1 - Add an RAII class to JSD to save/restore exception state. v1 (1.58 KB, patch)
2013-06-07 13:03 PDT, Bobby Holley (:bholley) (busy with Stylo)
gkrizsanits: review+
Details | Diff | Splinter Review
Part 2 - Stop using clunky C API in JSD and start using RAII classes. v1 (23.91 KB, patch)
2013-06-07 13:03 PDT, Bobby Holley (:bholley) (busy with Stylo)
gkrizsanits: review+
Details | Diff | Splinter Review
Part 3 - Make JS_ClearScriptTraps take a runtime directly. v1 (2.53 KB, patch)
2013-06-07 13:03 PDT, Bobby Holley (:bholley) (busy with Stylo)
gkrizsanits: review+
Details | Diff | Splinter Review
Part 4 - Include XPConnect and grab the SafeJSContext when nsContentUtils has been torn down. v1 (2.00 KB, patch)
2013-06-07 13:04 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
Part 5 - Replace usage of dumbContext with AutoSafeJSContext. v1 (16.33 KB, patch)
2013-06-07 13:04 PDT, Bobby Holley (:bholley) (busy with Stylo)
gkrizsanits: review+
Details | Diff | Splinter Review
Part 6 - Remove JSD_GetDefaultJSContext and fix callers. v1 (11.39 KB, patch)
2013-06-07 13:05 PDT, Bobby Holley (:bholley) (busy with Stylo)
gkrizsanits: review+
Details | Diff | Splinter Review
Part 7 - Remove dumbContext. v1 (5.72 KB, patch)
2013-06-07 13:05 PDT, Bobby Holley (:bholley) (busy with Stylo)
gkrizsanits: review+
Details | Diff | Splinter Review
Part 8 - Push in a few other suspicious places. v1 (4.13 KB, patch)
2013-06-07 13:05 PDT, Bobby Holley (:bholley) (busy with Stylo)
gkrizsanits: review+
Details | Diff | Splinter Review
v0: Add constructor for Rooted<>(JSRuntime*) (7.46 KB, patch)
2013-06-07 18:28 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
Part 0 - Add constructor for Rooted<>(JSRuntime*). v1 (8.43 KB, patch)
2013-06-07 18:48 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
Part 4 - Root directly with a runtime in jsd_DestroyScriptHookProc. v1 (943 bytes, patch)
2013-06-07 19:08 PDT, Bobby Holley (:bholley) (busy with Stylo)
gkrizsanits: review+
Details | Diff | Splinter Review
Part 0 - Add constructor For Rooted<>(JSRutnime *). v2 (7.59 KB, patch)
2013-06-11 10:32 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
add JSRuntime ctor for Rooted (3.87 KB, patch)
2013-06-11 12:03 PDT, Bill McCloskey (:billm)
terrence.d.cole: review+
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2013-06-07 08:27:00 PDT
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] 2013-06-07 09:18:06 PDT
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) (busy with Stylo) 2013-06-07 09:24:42 PDT
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) (busy with Stylo) 2013-06-07 13:01:28 PDT
https://tbpl.mozilla.org/?tree=Try&rev=7287732f7bf5
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2013-06-07 13:03:35 PDT
Created attachment 759904 [details] [diff] [review]
Part 1 - Add an RAII class to JSD to save/restore exception state. v1
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2013-06-07 13:03:46 PDT
Created attachment 759905 [details] [diff] [review]
Part 2 - Stop using clunky C API in JSD and start using RAII classes. v1
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2013-06-07 13:03:59 PDT
Created attachment 759906 [details] [diff] [review]
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) (busy with Stylo) 2013-06-07 13:04:12 PDT
Created attachment 759907 [details] [diff] [review]
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) (busy with Stylo) 2013-06-07 13:04:28 PDT
Created attachment 759908 [details] [diff] [review]
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) (busy with Stylo) 2013-06-07 13:05:09 PDT
Created attachment 759909 [details] [diff] [review]
Part 6 - Remove JSD_GetDefaultJSContext and fix callers. v1
Comment 10 Bobby Holley (:bholley) (busy with Stylo) 2013-06-07 13:05:21 PDT
Created attachment 759910 [details] [diff] [review]
Part 7 - Remove dumbContext. v1

Whew.
Comment 11 Bobby Holley (:bholley) (busy with Stylo) 2013-06-07 13:05:41 PDT
Created attachment 759911 [details] [diff] [review]
Part 8 - Push in a few other suspicious places. v1
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2013-06-07 17:36:54 PDT
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] 2013-06-07 18:28:09 PDT
Created attachment 760029 [details] [diff] [review]
v0: Add constructor for Rooted<>(JSRuntime*)
Comment 14 Bobby Holley (:bholley) (busy with Stylo) 2013-06-07 18:36:40 PDT
Awesome, thanks Terrence!
Comment 15 Terrence Cole [:terrence] 2013-06-07 18:48:21 PDT
Created attachment 760032 [details] [diff] [review]
Part 0 - Add constructor for Rooted<>(JSRuntime*). v1

Actually add the new test.
Comment 16 Bobby Holley (:bholley) (busy with Stylo) 2013-06-07 19:08:05 PDT
Created attachment 760037 [details] [diff] [review]
Part 4 - Root directly with a runtime in jsd_DestroyScriptHookProc. v1
Comment 17 Bobby Holley (:bholley) (busy with Stylo) 2013-06-07 19:08:21 PDT
https://tbpl.mozilla.org/?tree=Try&rev=7c809db43d90
Comment 18 Gabor Krizsanits [:krizsa :gabor] 2013-06-10 03:52:20 PDT
(In reply to Bobby Holley (:bholley) from comment #0)
> Sigh.

Indeed...
Comment 19 Gabor Krizsanits [:krizsa :gabor] 2013-06-10 04:02:14 PDT
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 [:krizsa :gabor] 2013-06-10 04:05:04 PDT
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 [:krizsa :gabor] 2013-06-10 04:09:18 PDT
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 [:krizsa :gabor] 2013-06-10 04:13:55 PDT
Comment on attachment 759910 [details] [diff] [review]
Part 7 - Remove dumbContext. v1

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

\o/
Comment 23 Gabor Krizsanits [:krizsa :gabor] 2013-06-10 04:19:18 PDT
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 (:billm) 2013-06-10 15:13:10 PDT
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] 2013-06-11 10:32:59 PDT
Created attachment 761018 [details] [diff] [review]
Part 0 - Add constructor For Rooted<>(JSRutnime *). v2
Comment 26 Bill McCloskey (:billm) 2013-06-11 12:03:00 PDT
Created attachment 761076 [details] [diff] [review]
add JSRuntime ctor for Rooted

Sorry, this is what I meant.
Comment 27 Terrence Cole [:terrence] 2013-06-11 12:09:04 PDT
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) (busy with Stylo) 2013-06-11 21:14:48 PDT
(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) (busy with Stylo) 2013-06-11 21:17:27 PDT
https://tbpl.mozilla.org/?tree=Try&rev=a3cf185fe73d
Comment 30 Bobby Holley (:bholley) (busy with Stylo) 2013-06-12 09:53:26 PDT
https://tbpl.mozilla.org/?tree=Try&rev=19827850b9a8

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