Closed
Bug 513160
Opened 15 years ago
Closed 15 years ago
TR::getThis generates do-nothing code when this can't be the global
Categories
(Core :: JavaScript Engine, defect, P2)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta1-fixed |
status1.9.1 | --- | ? |
People
(Reporter: jorendorff, Assigned: mrbkap)
References
Details
(Whiteboard: [sg:critical] fixed-in-tracemonkey)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•15 years ago
|
||
We've gone back and forth a dozen times now about whether this is a security hole. I believe it is. The ins_choose calls at the end of TR::getThis are no-ops if `this` is not the global object at record time. But we should be guarding against `this` later being the inner window (which can happen various ways -- trust us). Blake is going to write a bunch of trace-tests for this.
Group: core-security
Whiteboard: [sg:critical]
Reporter | ||
Comment 2•15 years ago
|
||
The fix might be to leave fp->thisp null until its first use, then call js_ComputeThis, instead of temporarily populating it with the global object and using JSFRAME_COMPUTED_THIS. IIUC, JSOP_THIS would then be able to emit trivial code (just a stack load from fp->thisp) if thisp is not null at record time (because a TT_OBJECT stack slot will never be null at run time; we already guarded on that).
Comment 5•15 years ago
|
||
Kind of a red herring, but .. the do-nothing code that Jason spotted looks like this eq8 = eq ld4, inner cmov3 = cmov eq8 ? inner : ld4 cmov4 = cmov eq8 ? inner : cmov3 Adding the following rewrite rules in ExprFilter::ins3 (y == x) ? x : y => y (x == y) ? x : y => y reduces the instruction count on v8-richards from 3499 million to 3227 million. So it would be extremely cool (positively freezing) if the fix to this bug also had the same effect.
Comment 6•15 years ago
|
||
Note also, the numbers in comment #5 first require application of the patch attached to bug 504587 comment #96.
Reporter | ||
Comment 7•15 years ago
|
||
Blake and I had a long conversation about this. 1. Blake confirmed you can a reference to the inner object can escape this way. (Incidentally, the shell should have a way to check and see if you've got the inner object or not--Blake looked in the debugger.) 2. The plan in comment 2 would make us trace the loop twice, once with thisp:TT_NULL and once with thisp:TT_OBJECT. 3. Blake's counteroffer was: this_ins = insChoose(getfslot(thisp_ins, PARENT), thisp_ins, INSCONST(wrapped outer window)) which is fine but adds a scary new invariant that the only parentless object that can ever be "really exposed to script" is the parent object. 4. I think we have a better plan, about which more in a moment.
Reporter | ||
Comment 8•15 years ago
|
||
The plan is: compute fp->thisp eagerly so the slot is always correct. JSOP_THIS should just do PUSH(fp->thisp). I think we used to do that but it was slow. The JS_ComputeThis machinery is an attempt to optimize via laziness. Instead we should cache. The pobj calculated by js_FindPropertyHelper (and js_FullTestPropertyCache) should be an object that can be used as thisp without any further fuss. That means unwrapping With objects, replacing Call and Block objects with the global, and outerizing split global objects. The trick is to make the property cache cover this work. If (innerWindow -> wrappedOuterWindow) is a fixed relation, we can cache the end-to-end result of the whole lookup, both pobj-as-ammended and sprop. It will require some hacking, maybe a new kind of JSPropCacheEntry or some out-of-line data. JS_ComputeThis would go away, which would save us a function call on most fast natives. JIT would win, interpreter might win big, and it seems simpler to me overall than what we're doing.
Comment 10•15 years ago
|
||
Jason, great thinking -- JSFastNative and JS_THIS came in before the return of the property cache, and we didn't think to use it instead of the COMPUTED_THIS frame flag and laziness. You don't need any new propcache entry, just use an object-valued entry keyed by JSOP_THIS etc. /be
Comment 11•15 years ago
|
||
Comment on attachment 397303 [details] [diff] [review] patch to remove silly cmov cases as per comment #5 patch is obsoleted by "patch v2" on bug 513407
Attachment #397303 -
Attachment is obsolete: true
Assignee | ||
Comment 12•15 years ago
|
||
I'll file a followup bug on making computing this be BLAZINGLY FAST.
Reporter | ||
Comment 14•15 years ago
|
||
Comment on attachment 399603 [details] [diff] [review] Immediate fix for this bug >diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp In TraceRecorder::getThis: >+ JSObject* wrappedGlobal = globalObj->thisObject(cx); >+ if (!wrappedGlobal) >+ return JSRS_ERROR; ABORT_TRACE_ERROR("helpful message"); This also wants a test. I think you were able to get hold of an inner object in the shell using this bug, so a trace-test seems appropriate. r+ with those changes.
Attachment #399603 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 16•15 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/2b8f660c66cd
Whiteboard: [sg:critical] → [sg:critical] fixed-in-tracemonkey
Assignee | ||
Comment 17•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/2b8f660c66cd
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 18•15 years ago
|
||
If this is a security bug should it also be fixed in 1.9.2, or was it introduced after that branch? Does it need to be fixed in 1.9.1 (Firefox 3.5) also? I feel more confident in assuming it doesn't affect 1.9.0
Assignee | ||
Comment 19•15 years ago
|
||
Yeah, we should fix this on all relevant branches (which, afaict, is 1.9.1 and 1.9.2).
Updated•15 years ago
|
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Comment 20•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5ffab6d66686
status1.9.2:
--- → beta1-fixed
Updated•13 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•