Closed
Bug 725161
Opened 13 years ago
Closed 13 years ago
js::StackFrame::getValidCalleeObject can create a bad cloned method
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
People
(Reporter: jorendorff, Assigned: dmandelin)
References
Details
(Whiteboard: [sg:critical][advisory-tracking+])
Attachments
(2 files)
119.30 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
135.57 KB,
patch
|
Details | Diff | Splinter Review |
This is a little hard to explain. First you have to know what the method optimization is.
When we trip the method barrier, we don't go and immediately fix up all calleev slots on the stack. We create the Function object and write it back to the method slot, but some calleev stack slots may still refer to the uncloned function object. getValidCalleeObject tries to repair them on demand, but it's fairly easy to defeat that code.
Test case 1 shows this happening; the assignment "obj.test = 12" is enough to make it impossible to fix the stack slot when we ask for f.caller. This test case just shows a minor correctness bug, probably unimportant.
But the fallback code in getValidCalleeObject, once the repair code is defeated, clones the method to a rather arbitrarily chosen global. It could end up being cloned to a wrong global, which could be crashy. This calls for a separate test case which I don't have time to put together tonight.
Can't take this immediately.
// Test case 1
var a, b;
function f() { return f.caller; }
var obj = {
test: function() {
a = obj.test;
obj.test = 12;
b = f();
}
};
obj.test();
assertEq(a, b);
Reporter | ||
Comment 1•13 years ago
|
||
Security consequences: Need to ask someone who understands the compile-n-go global-specific optimizations. Possibly reading fairly arbitrary heap locations, possibly writing to them? Don't know.
Comment 2•13 years ago
|
||
This sounds like sg:critical based on comment 1. If it turns out not to be as bad as guessed there, we can easily downgrade this once it's better understood.
Whiteboard: [sg:critical]
Updated•13 years ago
|
status-firefox10:
--- → wontfix
status-firefox11:
--- → wontfix
status-firefox12:
--- → affected
status-firefox13:
--- → affected
tracking-firefox10:
--- → -
tracking-firefox11:
--- → -
tracking-firefox12:
--- → +
tracking-firefox13:
--- → +
Assignee | ||
Comment 3•13 years ago
|
||
This was just reported, and it's way too subtle to fix for 12. 13 is a good target.
Assignee | ||
Comment 4•13 years ago
|
||
Actually, now that I think about it more (and talk with Luke), I think the problem is basically because the method read barrier is super-twitchy, and we could investigate this by turning off that optimization. It looks like it would be enough to (a) not make functions joinable (i.e., cut off all calls to setJoinable) and (b) not emit JSOP_INITMETHOD. This is not quite trivial because there are a few different places in the front end that set those values, but it should not be that hard. Turning off the optimization should fix this bug, so we'd just want to check that it doesn't regress perf too much. It would be great to land the turning-off before the next cutoff, but I still don't think we want to rush any fix out for 12.
Comment 5•13 years ago
|
||
Assigning to dmandelin so we don't lose track of the plan in comment 4. Please reassign as appropriate. If you don't want to rush a cycle end then it might be good to land early in Fx 14 (i.e. soon) for feedback.
Assignee: general → dmandelin
status-firefox-esr10:
--- → affected
status-firefox14:
--- → affected
tracking-firefox14:
--- → +
Assignee | ||
Comment 6•13 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #5)
> Assigning to dmandelin so we don't lose track of the plan in comment 4.
> Please reassign as appropriate. If you don't want to rush a cycle end then
> it might be good to land early in Fx 14 (i.e. soon) for feedback.
I may not be able to get to this before next week, but I'll try to get to it fairly soon, since we do need some time to see if the deoptimization doesn't hurt and shake out any regressions.
Assignee | ||
Comment 7•13 years ago
|
||
Patch to disable mb running on try:
https://tbpl.mozilla.org/?tree=Try&rev=58a9a22fb868
It passes shell tests, now need to check browser tests and perf characteristics.
Assignee | ||
Comment 8•13 years ago
|
||
OK, it's looking good on try: just one test failure, which is a test that needs fixing; Talos numbers are good; and gmail memory usage is unaffected. I'll start working on the real patch now.
Assignee | ||
Comment 9•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #609580 -
Flags: review?(luke)
Comment 10•13 years ago
|
||
Comment on attachment 609580 [details] [diff] [review]
Patch
Review of attachment 609580 [details] [diff] [review]:
-----------------------------------------------------------------
Nice job carving this turkey. I believe you can remove TCF_FUN_USES_OWN_NAME too.
::: js/src/frontend/BytecodeEmitter.cpp
@@ +5354,1 @@
> if (!wantval &&
nuke
::: js/src/jsfun.cpp
@@ +102,2 @@
> {
> if (!isFunctionFrame()) {
There seem to be 3 uses of getValidCalleeObject, all of which would benefit from inlining its remaining body (there are already isFunctionFrame-ish guards which would be merged with this one). Then we could really salt the earth and remove getValidCalleeObject.
::: js/src/jsobj.cpp
@@ +4418,5 @@
> Shape *
> js_ChangeNativePropertyAttrs(JSContext *cx, JSObject *obj,
> Shape *shape, unsigned attrs, unsigned mask,
> PropertyOp getter, StrictPropertyOp setter)
> {
You can inline this into its 1 caller, then inline its caller into its two callers (which will produce nice symmetric expressions) and remove these two functions (and their 2 decls in jsobj.h, one of which mentions "locking" in the comment ;).
::: js/src/methodjit/StubCalls.h
@@ -61,5 @@
> void JS_FASTCALL Interrupt(VMFrame &f, jsbytecode *pc);
> void JS_FASTCALL RecompileForInline(VMFrame &f);
> void JS_FASTCALL InitElem(VMFrame &f, uint32_t last);
> void JS_FASTCALL InitProp(VMFrame &f, PropertyName *name);
> -void JS_FASTCALL InitMethod(VMFrame &f, PropertyName *name);
There are a few more decls to purge here (LambdaJoinable*).
Attachment #609580 -
Flags: review?(luke) → review+
Assignee | ||
Comment 11•13 years ago
|
||
Assignee | ||
Comment 13•13 years ago
|
||
Fixed by the landing of bug 739808. The patch is huge and the bug obscure, so I'm not going to nominate for branches.
Assignee | ||
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WORKSFORME
Comment 14•12 years ago
|
||
Marking fixed for 14 transitively from bug 739808.
Assignee | ||
Comment 15•12 years ago
|
||
I have a smaller patch for beta that just disables the optimization without doing all the cleanup. It should backport to ESR10 fairly easily. I'm running it on try now.
Comment 16•12 years ago
|
||
Is there an ETA for the ESR fix to land for this?
Whiteboard: [sg:critical] → [sg:critical][advisory-tracking+]
Comment 17•12 years ago
|
||
If this is fixed by bug 739808 landing and that landed for ESR, why isn't this fixed on ESR?
Updated•12 years ago
|
tracking-firefox-esr10:
--- → ?
Updated•12 years ago
|
Updated•12 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•