630471 - Compartment mismatch in XPCJSStackFrame::CreateStack

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

I added a bunch of compartment checks to jsdbgapi.cpp, and they find a bunch of issues, but I'm not entirely sure which of these checks are legit.

I'll attach a pair of patches, and describe my issues further with them.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Add compartment checks all over jsdbgapi.cpp

This patch blindly adds compartment checks pretty much everywhere in jsdbgapi.cpp. With it, I get asserts on startup.

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Try 2: Remove some of the compartment checks from jsdbgapi.cpp

When digging through what the previous patch kicked up, I found some cases that I wasn't sure were really a problem, or that I just didn't know what to do with.

For example, js_UntrapScriptCode is called during GC, and the usual JSAutoEnterCompartment doesn't work within a GC run. So that assertion probably isn't a good idea.

I also wasn't sure about the things that are basically simple accessors for fields within an object, eg JS_GetScriptPrincipals. They seem like they might be useful to have available in situations where you may not be in the right compartment. On the other hand, if compartments are (or will be) used for lock-free threading zones, then none of them are safe.

Finally, one set of things I wasn't sure about is stuff like JS_PCToLineNumber. xpconnect uses this to generate stack traces, so it iterates over the current set of stack frames and invokes it once per frame. Switching compartments means pushing dummy stack frames, and I don't know if it works to be mutating the stack while you're iterating over it.

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Fix: XPCJSStackFrame::CreateStack

Here's an example of a fix triggered by these asserts. It's actually a very minor change, but I couldn't deal with increasing a function with 9 levels of indention to 10 levels just to add the compartment check, so I flattened it out with gotos as well.

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Fix: nsScriptSecurityManager::GetFramePrincipal

Another example fix -- enter a compartment before reading off principals. I'm unsure whether this is valid or necessary, and it's scary code to be changing.

Comment 5

[Steve Fink [:sfink] [:s:]]

I think I found bug 629822, but at least I know it's a real bug.

All of these are things I encountered when attempting to run the Firebug test suite (using the fbtest extension), but as you can see some of them are independent of JSD. (I have another set of fixes for JSD compartment mismatches uncovered this way, but I have those hanging off of bug 628758. Which I need to update.)

Comment 6

[Andreas Gal :gal]

Comment on attachment 508687 [details] [diff] [review]
Fix: XPCJSStackFrame::CreateStack

The goto is ugly. Maybe an Auto helper?

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Enter compartment, with RAII

Not only is the goto ugly, it introduces a memory leak.

Here's an RAII version, but the cleanup class really ought to be somewhere else, not stuck here. So I'll post a simpler version next for you to choose from.

Comment 8

[Steve Fink [:sfink] [:s:]]

Attachment: Enter compartment, simple if

It got too complicated, so here's the simple version that just adds a level of indent. Pick one.

Comment 9

[Steve Fink [:sfink] [:s:]]

Comment on attachment 508688 [details] [diff] [review]
Fix: nsScriptSecurityManager::GetFramePrincipal

In talking to gal, it sounds like this one is unnecessary. I did a scan of the JSAPI calls accessed from here:

JS_ObjectIsFunction(cx, obj)
GET_FUNCTION_PRIVATE(cx, obj)
JS_GetFunctionScript(cx, fun)
JS_GetFrameScript(cx, fp)
JS_GetFunctionObject(fun)
JS_GetFrameFunctionObject
JS_GetScriptPrincipals

I can believe those are all safe, using the test "can it run code or set an exception on the context"?

Comment 10

[Luke Wagner [:luke]]

Comment on attachment 509165 [details] [diff] [review]
Enter compartment, with RAII

Holy Righteous RAII, Batman!

>+    if(self->IsJSFrame()) {
>+        JSScript* script = JS_GetFrameScript(cx, fp);
>+        jsbytecode* pc = JS_GetFramePC(cx, fp);
>+        if(script && pc) {
>+            JSAutoEnterCompartment ac;
>+            if(!ac.enter(cx, script))
>             {

Should this ac's scope enclose the preceding JS_GetFrameScript/JS_GetFramePC?

Comment 11

[Steve Fink [:sfink] [:s:]]

(In reply to comment #10)
> Comment on attachment 509165 [details] [diff] [review]
> Enter compartment, with RAII
> 
> Holy Righteous RAII, Batman!
> 
> >+    if(self->IsJSFrame()) {
> >+        JSScript* script = JS_GetFrameScript(cx, fp);
> >+        jsbytecode* pc = JS_GetFramePC(cx, fp);
> >+        if(script && pc) {
> >+            JSAutoEnterCompartment ac;
> >+            if(!ac.enter(cx, script))
> >             {
> 
> Should this ac's scope enclose the preceding JS_GetFrameScript/JS_GetFramePC?

Using what to provide the scope object? I suppose I could at least put the JS_GetFramePC within the ac's scope, but they're both just field accessors.

But never mind. I don't really want to have the AutoNSCleanup class here. If something similar were already defined in nsISupportsUtils.h, I'd use it, but I don't really want to put something locally here. I'll just use the other patch, even with its 10 levels of nesting.

Comment 12

[Steve Fink [:sfink] [:s:]]

Clearing out my queue. Stuff to make remaining problems easier to find.

I'll move the assertions into a new bug.

http://hg.mozilla.org/tracemonkey/rev/d461afeeae3d

Comment 13

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/d461afeeae3d

Comment 14

[timeless]

sfink: https://hg.mozilla.org/mozilla-central/rev/d461afeeae3d

the braces in this blob are misplaced:
    1.19 +                    JSAutoEnterCompartment ac;
    1.20 +                    if(ac.enter(cx, script))
    1.21 +                     {
    1.22 +                         const char* filename = JS_GetScriptFilename(cx, script);
    1.23 +                         if(filename)
    1.24 +                        {
    1.25 +                            self->mFilename = (char*)
    1.26 +                                    nsMemory::Clone(filename,
    1.27 +                                            sizeof(char)*(strlen(filename)+1));
    1.28 +                        }

It'd be nice if this was fixed (as its own commit) at some point.

-- found because this commit causes a merge conflict for http://hg.mozilla.org/users/timeless_mozdev.org/mozilla-central-mq/file/default/538143-XPCJSStackFrame-CreateStack-no-recursion

Comment 15

[timeless]

(technically the first brace is misplaced, the second brace is correctly placed but its contents is misplaced)