Closed
Bug 630471
Opened 13 years ago
Closed 13 years ago
Compartment mismatch in XPCJSStackFrame::CreateStack
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: sfink, Unassigned)
References
Details
(Whiteboard: [softblocker][fixed-in-tracemonkey])
Attachments
(4 files, 2 obsolete files)
12.57 KB,
patch
|
Details | Diff | Splinter Review | |
5.31 KB,
patch
|
Details | Diff | Splinter Review | |
5.40 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
3.36 KB,
patch
|
gal
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•13 years ago
|
||
This patch blindly adds compartment checks pretty much everywhere in jsdbgapi.cpp. With it, I get asserts on startup.
Reporter | ||
Comment 2•13 years ago
|
||
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.
Reporter | ||
Comment 3•13 years ago
|
||
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.
Reporter | ||
Comment 4•13 years ago
|
||
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.
Reporter | ||
Comment 5•13 years ago
|
||
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.)
Depends on: 629822
Comment 6•13 years ago
|
||
Comment on attachment 508687 [details] [diff] [review] Fix: XPCJSStackFrame::CreateStack The goto is ugly. Maybe an Auto helper?
Attachment #508687 -
Flags: review+
Reporter | ||
Comment 7•13 years ago
|
||
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.
Attachment #508687 -
Attachment is obsolete: true
Attachment #509165 -
Flags: review?(gal)
Reporter | ||
Comment 8•13 years ago
|
||
It got too complicated, so here's the simple version that just adds a level of indent. Pick one.
Attachment #509166 -
Flags: review?(gal)
Reporter | ||
Comment 9•13 years ago
|
||
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"?
Attachment #508688 -
Attachment is obsolete: true
Updated•13 years ago
|
Attachment #509165 -
Flags: review?(gal) → review?(lw)
Updated•13 years ago
|
Attachment #509166 -
Flags: review?(gal) → review+
Comment 10•13 years ago
|
||
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?
Attachment #509165 -
Flags: review?(lw) → review+
Reporter | ||
Comment 11•13 years ago
|
||
(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.
Reporter | ||
Updated•13 years ago
|
blocking2.0: --- → ?
Updated•13 years ago
|
blocking2.0: ? → betaN+
Whiteboard: [softblocker]
Reporter | ||
Comment 12•13 years ago
|
||
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
Summary: Compartment mismatches in jsdbgapi.cpp → Compartment mismatch in XPCJSStackFrame::CreateStack
Whiteboard: [softblocker] → [softblocker][fixed-in-tracemonkey]
Comment 13•13 years ago
|
||
cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/d461afeeae3d
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 14•13 years ago
|
||
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•13 years ago
|
||
(technically the first brace is misplaced, the second brace is correctly placed but its contents is misplaced)
You need to log in
before you can comment on or make changes to this bug.
Description
•