Closed Bug 627984 Opened 9 years ago Closed 9 years ago

watch() can make us miss the method write barrier

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla2.0
Tracking Status
firefox5 --- unaffected
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: jandem, Assigned: jorendorff)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [sg:nse][fixed-in-tracemonkey] hidden while bug 631723 is)

Attachments

(2 files)

This asserts in interpreter/-m/-j:
--
var o = Array;
o.p = function() {};
o.watch('p', function() { }); 
for(var x in o) { 
    o[x]; 
}
delete o.p;
o.p = function() {};
print(o.p);
--
Assertion failure: isObject(), at ../jsvalue.h:602
Attached file Stacktrace
blocking2.0: --- → ?
Reduced further:
--
var o = Array;
o.watch('p', function() { }); 
for(var x in o) {  };
delete o.p;
o.p = function() {};
print(o.p);
--
The first bad revision is:
changeset:   58560:52d20032116a
user:        Jason Orendorff <jorendorff@mozilla.com>
date:        Thu Dec 09 12:04:35 2010 -0600
summary:     Bug 614051 - TM: wrong behavior setting existing properties to joined function object values again. r=brendan.
Blocks: 614051
Keywords: regression
security sensitive per jandem's request.
Group: core-security
This one crashes with -j in a release build:
--
var o = Array;
o.watch('p', function() { }); 
for(var x in o) {};
delete o.p;
o.p = function() {};

for(var i=0; i<20; i++) {
    print(o.p);
}
--
Stack trace:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x0000001c
0x00194fea in js::MethodReadBarrier ()
(gdb) bt
#0  0x00194fea in js::MethodReadBarrier ()
#1  0x00382f08 in ?? ()
#2  0x001b326c in js::ExecuteTree ()
Previous frame inner to this frame (gdb could not unwind past this frame)
This seems relatively obscure, and seems not to be sg:critical, so that this late, I think it only needs to be a softblocker.
blocking2.0: ? → final+
Whiteboard: softblocker
Whiteboard: softblocker → softblocker, [sg:high]
The branded shape and the slot holding the method are out of sync here. This is at least pretty scary.
var o = Array;
o.p = function() {};
o.watch('p', function() { });
Object.keys(o);
delete o.p;
o.p = function() {};
print(o.p);

The iteration seems necessary here (keys), I don't know why.
Object.keys triggers the resolve hook on Array. This simplified test case crashes too:

var o = Array;
o.p = function() {};
o.watch('p', function() { });
o.length;
delete o.p;
o.p = function() {};
print(o.p);

Looks like we mess up the write barrier here. Watch sets a getter which makes shape no longer say yes to "isMethod", and resolving another method on that causes bad things. Pretty sketchy stuff.
No resolve hook needed. Just adding another function (which causes rebranding) will do it.

var o = ({});
o.p = function() {};
o.watch('p', function() { });
o.q = function() {}
delete o.p;
o.p = function() {};
print(o.p);
This is the state right before we hit the methodReadBarrier in print(o.p). The object has clearly not the right method value in the slot:

Breakpoint 4, JSObject::methodReadBarrier (this=0x100d0d068, cx=0x1008130e0, shape=@0x10111a5c8, vp=0x7fff5fbfede0) at jsobjinlines.h:165
165	    if (!js_SetPropertyHelper(cx, this, shape.id, 0, vp, false))
(gdb) call js_DumpObject(this)
object 0x100d0d068
class 0x100438ea0 Object
flags: method_barrier inDictionaryMode hasPropertyTable
properties:
    enumerate method(0x100d0bd80) "p": slot 2
    enumerate method(0x100d0bd00) "q": slot 1
proto <Object at 0x100d030d8>
parent <global object at 0x100d03048>
slots:
   0 = undefined
   1 = <unnamed function (x.js:4) at 0x100d0bd00 (JSFunction at 0x100d0bd00)>
   2 = undefined

(gdb)
We call the watch point in SetMethod after we have updated the shape and then write that value into the slot. The watch point returns undefined and that value is written. It might be possible to trick the property cache into calling the wrong function with this:

(gdb) s
obj_watch_handler (cx=0x1008130e0, obj=0x100d0d068, id={asBits = 4299215808}, old={asBits = 18444773748872577024, debugView = {payload47 = 0, tag = JSVAL_TAG_UNDEFINED}, s = {payload = {i32 = 0, u32 = 0, why = JS_ARRAY_HOLE, word = 18444773748872577024}}, asDouble = -nan(0x9000000000000), asPtr = 0xfff9000000000000}, nvp=0x7fff5fbfed90, closure=0x100d0e058) at ../jsobj.cpp:1310
1310	    callable = (JSObject *) closure;
(gdb) n
1312	    callbacks = JS_GetSecurityCallbacks(cx);
(gdb) n
1313	    if (callbacks && callbacks->findObjectPrincipals) {
(gdb) n
1332	    key.obj = obj;
(gdb) n
1333	    key.id = id;
(gdb) n
1334	    if (!js_StartResolving(cx, &key, JSRESFLAG_WATCH, &entry))
(gdb) n
1336	    if (!entry)
(gdb) n
1338	    generation = cx->resolvingTable->generation;
(gdb) n
1340	    argv[0] = IdToValue(id);
(gdb) n
1341	    argv[1] = Valueify(old);
(gdb) n
1342	    argv[2] = Valueify(*nvp);
(gdb) n
1343	    ok = ExternalInvoke(cx, obj, ObjectOrNullValue(callable), 3, argv, Valueify(nvp));
(gdb) p *nvp
$63 = {
  asBits = 18445477440623000960, 
  debugView = {
    payload47 = 4308647296, 
    tag = JSVAL_TAG_OBJECT
  }, 
  s = {
    payload = {
      i32 = 13680000, 
      u32 = 13680000, 
      why = 13680000, 
      word = 18445477440623000960
    }
  }, 
  asDouble = -nan(0xb800100d0bd80), 
  asPtr = 0xfffb800100d0bd80
}
(gdb) n
1344	    js_StopResolving(cx, &key, JSRESFLAG_WATCH, entry, generation);
(gdb) p *nvp
$64 = {
  asBits = 18444773748872577024, 
  debugView = {
    payload47 = 0, 
    tag = JSVAL_TAG_UNDEFINED
  }, 
  s = {
    payload = {
      i32 = 0, 
      u32 = 0, 
      why = JS_ARRAY_HOLE, 
      word = 18444773748872577024
    }
  }, 
  asDouble = -nan(0x9000000000000), 
  asPtr = 0xfff9000000000000
}
(gdb) 

(gdb) l
5662	                getter = CastAsPropertyOp(funobj);
5663	            }
5664	        }
5665	
5666	        shape = obj->putProperty(cx, id, getter, setter, SHAPE_INVALID_SLOT,
5667	                                 attrs, flags, shortid);
5668	        if (!shape)
5669	            return JS_FALSE;
5670	
5671	        if (defineHow & JSDNP_CACHE_RESULT)
(gdb) n
5668	        if (!shape)
(gdb) n
5671	        if (defineHow & JSDNP_CACHE_RESULT)
(gdb) n
5672	            TRACE_1(AddProperty, obj);
(gdb) l
5667	                                 attrs, flags, shortid);
5668	        if (!shape)
5669	            return JS_FALSE;
5670	
5671	        if (defineHow & JSDNP_CACHE_RESULT)
5672	            TRACE_1(AddProperty, obj);
5673	
5674	        /*
5675	         * Initialize the new property value (passed to setter) to undefined.
5676	         * Note that we store before calling addProperty, to match the order
(gdb) n
5679	        if (obj->containsSlot(shape->slot))
(gdb) n
5680	            obj->nativeSetSlot(shape->slot, UndefinedValue());
(gdb) n
5683	        if (!CallAddPropertyHook(cx, clasp, obj, shape, vp)) {
(gdb) p *vp
$54 = {
  data = {
    asBits = 18445477440623000960, 
    debugView = {
      payload47 = 4308647296, 
      tag = JSVAL_TAG_OBJECT
    }, 
    s = {
      payload = {
        i32 = 13680000, 
        u32 = 13680000, 
        why = 13680000, 
        word = 18445477440623000960
      }
    }, 
    asDouble = -nan(0xb800100d0bd80), 
    asPtr = 0xfffb800100d0bd80
  }
}
(gdb) call js_DumpValue(*vp)
<unnamed function (x.js:6) at 0x100d0bd80 (JSFunction at 0x100d0bd80)>
(gdb) s
CallAddPropertyHook (cx=0x1008130e0, clasp=0x100438ea0, obj=0x100d0d068, shape=0x10111a5c8, vp=0x7fff5fbfed90) at ../jsobj.cpp:4576
4576	    if (clasp->addProperty != PropertyStub) {
(gdb) n
4586	    return true;
(gdb) fin
Run till exit from #0  CallAddPropertyHook (cx=0x1008130e0, clasp=0x100438ea0, obj=0x100d0d068, shape=0x10111a5c8, vp=0x7fff5fbfed90) at ../jsobj.cpp:4586
0x0000000100106c62 in js_SetPropertyHelper (cx=0x1008130e0, obj=0x100d0d068, id={asBits = 4299215808}, defineHow=5, vp=0x7fff5fbfed90, strict=0) at ../jsobj.cpp:5683
5683	        if (!CallAddPropertyHook(cx, clasp, obj, shape, vp)) {
Value returned is $55 = true
(gdb) n
5687	        added = true;
(gdb) p *vp
$56 = {
  data = {
    asBits = 18445477440623000960, 
    debugView = {
      payload47 = 4308647296, 
      tag = JSVAL_TAG_OBJECT
    }, 
    s = {
      payload = {
        i32 = 13680000, 
        u32 = 13680000, 
        why = 13680000, 
        word = 18445477440623000960
      }
    }, 
    asDouble = -nan(0xb800100d0bd80), 
    asPtr = 0xfffb800100d0bd80
  }
}
(gdb) n
5690	    if (defineHow & JSDNP_CACHE_RESULT)
(gdb) n
5691	        JS_PROPERTY_CACHE(cx).fill(cx, obj, 0, 0, obj, shape, added);
(gdb) n
5693	    return js_NativeSet(cx, obj, shape, added, vp);
There is a bunch of bugs here actually. In js_NativeSet we never trigger the method write barrier if a property is added (added=true), because we think that can't cause a shape change, but that's not true for watchpoint setters which fire for additions as well since they are persistent. So in other words watch makes our write barrier more expensive in general. Super lame.

There is a second bug. The slot is initialized to undefined when we call the setter, so even if we would call the method write barrier it would see undefined (old) == undefined (new) and not brand.

I think this is all pretty seriously broken and all sorts of unsafe. I vote for a hard blocker. This probably wants jorendorff or brendan, though I think I can fix it if push comes to shove.
Summary: Assertion failure: isObject(), at ../jsvalue.h:602 → watch() can make us miss the method write barrier
Whiteboard: softblocker, [sg:high] → hardblocker, [sg:high]
Assignee: general → jorendorff
Blocks: 630996
Whiteboard: hardblocker, [sg:high] → [hardblocker], [sg:high]
What a mess.
Depends on: 631723
OK. My first instinct here was to trigger the method *read* barrier in UpdateWatchpointShape. If a method property would be watched, the barrier would convert it an ordinary data property. Then you would watch that.

But that would go seriously against the grain, because UpdateWatchpointShape gets called at a point where the method property has been added, but before the slot is filled.

So for now watchpoints need to know about the method barrier. (Which after all is only consistent. They know about everything else.)

I have a sort of patch, but unfortunately the assertions I'm trying to add keep failing, and I'm out of time for the day.
Elaborating a little on comment 16:

There are three bugs I know very clearly how to fix.

1. js_watch_set passes the property's old slot value (the one we're about to overwrite) to wp->handler without checking to see if the property is a method. If it's a method, the old value is an uncloned function object that must not be exposed to script! The fix is for js_watch_set to trip the read barrier before calling wp->handler.

2. js_watch_set can get called at an unfortunate time. The order of adding a data property, such as a method, is like this:
    - add the shape to the object
    - call the setter
    - store the value in the slot

So suppose we watch a property, then delete it, and then use JSOP_INITMETHOD to create it again. Here's what happens:
    - add the (watched method) shape to the object
    - call js_watch_set, which calls wp->handler
    - store the uncloned function object in the slot

Note js_watch_set is called *after* the METHOD property is added to obj but *before* the slot is assigned. So the slot value is undefined. To my mind, this state is a violation of the method invariant. We do not want to be calling back into the interpreter with the object in that state. The fix, again, is for js_watch_set to trip the method barrier before calling wp->handler.

3. The watchpoint handler could delete the property and re-add it as a method property again. Then whatever value it returns must not be stored in the slot of that method property; it would violate the method invariant. So after the handler returns, we probably need to call methodWriteBarrier. (This one I'm not quite sure about.)

There are also at least 2 other issues:

4. Bug 631723 - Deleted watchpoints can make obj->addProperty/putProperty return a shape not in obj.

5. If wp->handler deletes the property, wp->shape is not set to NULL. I think I have seen wp->shape be wrong (that is, != obj->nativeLookup(propid)) in other circumstances, too, which would be bad. This needs another look, possibly in a follow-up bug.
6. js_NativeSet skips the slot write if the before and after shape of the property are different. This means that unwatching from a watchpoint can cause the slot write to be skipped:

var obj = {m: 'o'};
var other = {a: 1};
obj.watch("m", function (id, oldval, newval) { 
        delete other.a;
        obj.unwatch('m');
        return newval;
    });
obj.m = 'n';  // since the watchpoint unconditionally returns newval,
              // this value should not be lost
assertEq(obj.m, 'n');  // FAILS

Similarly, scripts can detect the transition to dictionary mode (which re-creates all Shapes) this way.

var N = 129;
var obj = {m: "fail"};
var other = {a: 1};
obj.watch("m", function (id, oldval, newval) {
        for (var j = 0; j < N; j++)
            obj['x' + j] = 0;
        delete other.a;
        return newval;
    });
obj.m = 'ok';
assertEq(obj.m, 'ok');  // FAILS when N >= 128
To clarify a little on comment 17 item 2,
Depends on: 631305
Kindly disregard comment 19. My clarification wasn't going to be correct anyway. (If item 2 isn't clear, wait for the patch -- there's a test.)

I found another bug.

7. A method is ostensibly an ordinary data property--but getting its value the first time will trigger a watchpoint on that property.

  var obj = {m: function () {}};
  obj.watch("m", function () { throw 'FAIL'; });
  var f = obj.m;  // FAIL

Patch coming!
Attached patch v1Splinter Review
This patch applies on top of the patches in bug 631723 and bug 631305.

This patch does not address comment 17 item 5 or comment 18 item 6. I will file follow-up bugs for those.
Attachment #510780 - Flags: review?(brendan)
Whiteboard: [hardblocker], [sg:high] → [hardblocker], [sg:high][has patch]
Wow, what a mess indeed. Sorry I forgot about watchpoints when doing the method stuff.

Could you fix sevearl of these bugs before the js_watch_set setter is invoked, in js_UpdateWatchpointsForShape? Joined methods are an optimization, so deoptimizing as soon as there's sign of a watchpoint seems fine, and it looks from this patch like it would be simpler.

/be
blocking2.0: final+ → .x
Whiteboard: [hardblocker], [sg:high][has patch] → [sg:high][has patch]
Brendan and I talked about this on IRC yesterday or the day before.

Two reasons not to do it that way: first, js_UpdateWatchpointsForShape is called at a weird time; the method property exists but the slot isn't populated yet. No big deal; I could populate the slot from shape->methodValue(), then trip the read barrier.

Second, and more importantly, js_UpdateWatchpointsForShape's relevant caller (js_SetPropertyHelper) doesn't expect it to trip the read barrier, so it will unconditionally write the noncloned function to the slot. To avoid that, the caller would have to check whether js_UpdateWatchpointsForShape changed the shape. That adds a branch to the no-watchpoint path, and it complicates code outside jsdbgapi.cpp.

So I put the added complexity in js_watch_set, which seemed more consistent with the design.
Jason's right, the ancient watchpoint design, which predates user-defined setters, tries to modularize watchpoints by putting their overhead behind a setter wrapper. This hasn't aged particularly well. His preferred approach would add a few

    if (JS_UNLIKELY(obj->watched())) { // obj->flags & WATCHED
        . . .
    }

clauses. Sounds worth filing and doing after fx4.

/be
Comment on attachment 510780 [details] [diff] [review]
v1

>+                 * property added. We must write the slot ourselves--however we

Nit: spaces around --, should rewrap nicely.

>+                //JS_ASSERT_IF(shape, wp->shape == shape);
>+                //JS_ASSERT_IF(!shape, !wp->setter);

Uncomment (fix if needed?) or remove.

>+                    ok = (shape->hasSetterValue()
>+                          ? ExternalInvoke(cx, ObjectValue(*obj),
>+                                           ObjectValue(*CastAsObject(wp->setter)),
>+                                           1, vp, vp)
>+                          : CallJSPropertyOpSetter(cx, wp->setter, obj, userid, vp));

Super-duper-nit: no need to parenthesize the whole ?: expression.

>+                     * It is not ordinarily the setter's job to call
>+                     * methodWriteBarrier; js_watch_set alone must do so, because

Would wrap better if written

                    /*
                     * It is not the setter's job to call methodWriteBarrier,
                     * but js_watch_set must do so, because the caller will be
                     * fooled into not doing it: shape does *not* have the
                     * default setter and therefore seems not to be a method.
                     */

Great work, r=me. Thanks for diving on this grenade.

/be
Attachment #510780 - Flags: review?(brendan) → review+
I will land this as soon as bug 631723 gets review.

(In reply to comment #25)
> >+                //JS_ASSERT_IF(shape, wp->shape == shape);
> >+                //JS_ASSERT_IF(!shape, !wp->setter);
> 
> Uncomment (fix if needed?) or remove.

The second assertion passes the whole test suite, so I'll uncomment it.

The first assertion can be tripped, and not only that, I found another assertion in js_watch_set that can also be tripped. Filed bug 633637. I'm removing the trippable assertions from this patch for now.
http://hg.mozilla.org/tracemonkey/rev/589bb166be02
Whiteboard: [sg:high][has patch] → [sg:high][fixed-in-tracemonkey]
Depends on: 634210
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Given gal's analysis and several agreements that the code was a mess, is [sg:critical?] more accurate a severity rating?
status1.9.1: --- → ?
Whiteboard: [sg:high][fixed-in-tracemonkey] → [sg:critical?][fixed-in-tracemonkey]
The issues pointed out in comment 0 and ultimately fixed in this bug are correctness bugs, so this bug isn't sg:anything per se.

However several of the comments here, particularly comment 17 item 4, point to bug 631723, which is [sg:critical?]. So I'm not sure this should be opened.

(Given all that, I'm really not sure what should be done to this bug exactly.)
Depends on: 635531
Depends on: 636697
Whiteboard: [sg:critical?][fixed-in-tracemonkey] → [sg:nse][fixed-in-tracemonkey]
Whiteboard: [sg:nse][fixed-in-tracemonkey] → [sg:nse][fixed-in-tracemonkey] some comments point at other sec bugs
blocking2.0: .x+ → ---
Whiteboard: [sg:nse][fixed-in-tracemonkey] some comments point at other sec bugs → [sg:nse][fixed-in-tracemonkey] hidden while bug 631723 is
Target Milestone: --- → mozilla2.0
Group: core-security
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/extensions/regress-627984-6.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.