513160 - TR::getThis generates do-nothing code when this can't be the global

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Comment 1

[Jason Orendorff [:jorendorff]]

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.

Comment 2

[Jason Orendorff [:jorendorff]]

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 3

[Andreas Gal :gal]

#2 is pretty clever. I like it.

Comment 4

[Brendan Eich [:brendan]]

I <3 TT_NULL :-).

/be

Comment 5

[Julian Seward [:jseward]]

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

[Julian Seward [:jseward]]

Attachment: patch to remove silly cmov cases as per comment #5

Note also, the numbers in comment #5 first require application of
the patch attached to bug 504587 comment #96.

Comment 7

[Jason Orendorff [:jorendorff]]

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.

Comment 8

[Jason Orendorff [:jorendorff]]

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 9

[Andreas Gal :gal]

If we can cache it, we can burn it onto trace. That will be nice and fast.

Comment 10

[Brendan Eich [:brendan]]

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

[Julian Seward [:jseward]]

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

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Immediate fix for this bug

I'll file a followup bug on making computing this be BLAZINGLY FAST.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

That would be bug 515496.

Comment 14

[Jason Orendorff [:jorendorff]]

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.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

bug 516826 blocks adding a test for this.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/2b8f660c66cd

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/2b8f660c66cd

Comment 18

[Daniel Veditz [:dveditz]]

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

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Yeah, we should fix this on all relevant branches (which, afaict, is 1.9.1 and 1.9.2).

Comment 20

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5ffab6d66686