Last Comment Bug 725161 - js::StackFrame::getValidCalleeObject can create a bad cloned method
: js::StackFrame::getValidCalleeObject can create a bad cloned method
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: ---
Assigned To: David Mandelin [:dmandelin]
Depends on: 739808
  Show dependency treegraph
Reported: 2012-02-07 16:50 PST by Jason Orendorff [:jorendorff]
Modified: 2012-07-12 16:26 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (119.30 KB, patch)
2012-03-26 19:03 PDT, David Mandelin [:dmandelin]
luke: review+
Details | Diff | Review
Patch with suggested revisions (135.57 KB, patch)
2012-03-27 15:21 PDT, David Mandelin [:dmandelin]
no flags Details | Diff | Review

Description Jason Orendorff [:jorendorff] 2012-02-07 16:50:41 PST
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();
assertEq(a, b);
Comment 1 Jason Orendorff [:jorendorff] 2012-02-07 16:52:30 PST
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, 2012-02-29 17:07:44 PST
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] 2012-03-02 11:34:32 PST
This was just reported, and it's way too subtle to fix for 12. 13 is a good target.
Comment 4 David Mandelin [:dmandelin] 2012-03-02 11:57:43 PST
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] 2012-03-15 13:31:38 PDT
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] 2012-03-15 16:29:52 PDT
(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] 2012-03-23 18:17:37 PDT
Patch to disable mb running on try:

It passes shell tests, now need to check browser tests and perf characteristics.
Comment 8 David Mandelin [:dmandelin] 2012-03-26 17:09:48 PDT
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] 2012-03-26 19:03:32 PDT
Created attachment 609580 [details] [diff] [review]
Comment 10 Luke Wagner [:luke] 2012-03-27 09:54:26 PDT
Comment on attachment 609580 [details] [diff] [review]

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 &&


::: 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] 2012-03-27 15:21:44 PDT
Created attachment 609911 [details] [diff] [review]
Patch with suggested revisions
Comment 12 David Mandelin [:dmandelin] 2012-03-27 15:27:45 PDT
Filed bug 739808 as cover bug.
Comment 13 David Mandelin [:dmandelin] 2012-03-28 16:12:08 PDT
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] 2012-05-10 14:07:20 PDT
Marking fixed for 14 transitively from bug 739808.
Comment 15 David Mandelin [:dmandelin] 2012-05-10 18:39:55 PDT
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] 2012-07-09 14:56:28 PDT
Is there an ETA for the ESR fix to land for this?
Comment 17 Al Billings [:abillings] 2012-07-12 11:04:31 PDT
If this is fixed by bug 739808 landing and that landed for ESR, why isn't this fixed on ESR?

Note You need to log in before you can comment on or make changes to this bug.