725161 - js::StackFrame::getValidCalleeObject can create a bad cloned method

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

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);

Comment 1

[Jason Orendorff [:jorendorff]]

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

[Johnny Stenback (:jst)]

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.

Comment 3

[David Mandelin [:dmandelin]]

This was just reported, and it's way too subtle to fix for 12. 13 is a good target.

Comment 4

[David Mandelin [:dmandelin]]

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

[Daniel Veditz [:dveditz]]

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.

Comment 6

[David Mandelin [:dmandelin]]

(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.

Comment 7

[David Mandelin [:dmandelin]]

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.

Comment 8

[David Mandelin [:dmandelin]]

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.

Comment 9

[David Mandelin [:dmandelin]]

Attachment: Patch

Comment 10

[Luke Wagner [:luke]]

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*).

Comment 11

[David Mandelin [:dmandelin]]

Attachment: Patch with suggested revisions

Comment 12

[David Mandelin [:dmandelin]]

Filed bug 739808 as cover bug.

Comment 13

[David Mandelin [:dmandelin]]

Fixed by the landing of bug 739808. The patch is huge and the bug obscure, so I'm not going to nominate for branches.

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

Marking fixed for 14 transitively from bug 739808.

Comment 15

[David Mandelin [:dmandelin]]

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

[Al Billings [:abillings - ex-MoCo]]

Is there an ETA for the ESR fix to land for this?

Comment 17

[Al Billings [:abillings - ex-MoCo]]

If this is fixed by bug 739808 landing and that landed for ESR, why isn't this fixed on ESR?