561954 - TM: Make LeaveTraceIfGlobalObject abort recording

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Split off from bug 546611, if a native touches a global it'll trigger an assert. This could be very dangerous, if for example:

 1. Trace specializes global |X| as TT_INT32.
 2. Native call secretly changes global |X| to TT_OBJECT.
 3. Another native invocation uses |X|, the process of boxing up the stack
    will pass the integer as an object.

So we can't let the VM touch the global object behind the recorder.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

I can't think of any ways to exploit this right now. Setting a global inside something like lambda-replace won't trace because it re-enters the interpreter.

Comment 2

[Luke Wagner [:luke]]

Attachment: crash

I can get a crash as comment 0 describes by simulating such a write with a new shell function setGlobalPropIf.  As David pointed out to me, the trickier part is this: we can't simply

  if (TRACE_RECORDER(cx)) AbortRecording(cx)

since we expect to hit these during recording (the recorded code does not call the same function).  I think the solution is to only abort if the type of the global value changes.

Comment 3

[Luke Wagner [:luke]]

There are 17 calls to LeaveTraceIfGlobalObject.  The two that obviously allow the above problem are js_NativeSet and js_DefineNativeProperty.  Most of the others fall into two categories: those called from trace (where perhaps we can assert that we are not recording) and those called when an object changes shape.  I have to assume that we correctly handle shape changes while recording, is that right?

Comment 4

[Jason Orendorff [:jorendorff]]

(In reply to comment #3)
> [...] and those called when an object changes
> shape.  I have to assume that we correctly handle shape changes while
> recording, is that right?

Yes.

In those cases, we are calling LeaveTraceIfGlobalObject only because we never check the global object's shape on trace. For all other objects, we will actually emit a load again the next time we want to check the object's shape.

Comment 5

[Luke Wagner [:luke]]

Attachment: patch

Aborts recording in js_NativeGet and js_DefineNativeProperty.

Comment 6

[Luke Wagner [:luke]]

Attachment: patch

Please be suspicious of my placement of AbortRecordingIfChangingGlobalSlotType.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 491242 [details] [diff] [review]
patch

In shell/js.cpp:
>+    JS_FN("setGlobalPropIf",SetGlobalPropIf,2,0),
>+    JS_FN("defGlobalPropIf",DefGlobalPropIf,2,0),

Both take 3 arguments, right? Not that it matters.

>+"                         Object.defineProperty(o, id, dsc.\n"

Missing parenthesis in the help message.

I can live with AbortRecordingIfChangingGlobalSlotType where it is, but note that jstracer.h actually does have 

  #else  /* !JS_TRACER */
  ...
  #endif /* ! JS_TRACER */

toward the end. It's OK to put stuff in the #else or after it:

static JS_INLINE void
blah()
{
#ifdef JS_TRACER
...
#endif
}

and perhaps js_DeepBail and friends should be defined this way.

Comment 8

[Luke Wagner [:luke]]

Good point, much nicer in jstracer.h at the end.

Comment 9

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/31e0cd11f015

Comment 10

[Luke Wagner [:luke]]

Backed out:
http://hg.mozilla.org/tracemonkey/rev/31e0cd11f015

Despite running jittests clean locally, it manages to fail on TB.  Also, bizarrely, math-partial-sums gets 3x slower, but only when run with the full SS suite.  A global property is changing type...

Comment 11

[Luke Wagner [:luke]]

The jit-test failure was caused by the fact that we assume js_GetClassPrototype cannot abort recording (even assert it), but this patch causes that to happen when js_GetClassPrototype causes a global property to be lazily resolved.  That can be fixed...

The regression is caused by the following sequence:
 1. start recording math-partial-sums with an int
 2. branch exit at arithmetic op when int overflows to double
 3. start recording and assign the just-overflowed value to global slot, which causes a type change from int to double, which aborts

So this demonstrates something fundamental I missed in the original patch: js_NativeSet/js_DefineNativeProperty can be called in two context:
 1. the recorder records a global property set, then continues to the interpreter, which preforms the set;
 2. the recorder records a call to a native, then continues to the interpreter, which calls the native, which sets a global property.

(1) is ok even if the type changes since the recorder watched it happen, and this patch causes 1 to abort recording.  The fix seems to be:
 1. add a field to TraceRecorder: Value *expectedGlobalSet, initially null
 2. when recording a global property set, set expectedGlobalSet to the pointer to the global slot that is expected to change
 3. in js_NativeSet/js_DefineNativeProperty, abort recording if setting a property on the global != expectedGlobalSet. set expectedGlobalSet to null.
 4. assert in monitorRecording that expectedGobalSet is null

To actually expose/exploit this bug, content would need a native method analogous to SetGlobalPropIf.  I'm trying to find out now if such a native exists.

Comment 12

[Luke Wagner [:luke]]

Attachment: patch 2

Kinda like this.

Comment 13

[Luke Wagner [:luke]]

Attachment: patch 2

Additionally, we only abort if the global slot is "known" to the trace recorder.  This means that js_GetClassPrototype cannot abort recording since, if it is lazily resolving a global property, surely we are not tracking the modified slot (right?).

I verified that the added abort doesn't hit once during SS/V8/jit-tests, but does catch the global writes in the test case.

Comment 14

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/31e0cd11f015

Comment 15

[Jason Orendorff [:jorendorff]]

Comment on attachment 491965 [details] [diff] [review]
patch 2

We still trace the opcodes that use record_SetPropHit with this patch, right? The interpreter calls record_SetPropHit *before* setting the slot. Yes? Just checking.

In jstracer.cpp, TraceRecorder::scopeChainProp:
>         if (obj2 != obj)
>             RETURN_STOP_A("prototype property");
> 
>         Shape* shape = (Shape*) prop;
>         if (!isValidSlot(obj, shape))
>             return ARECORD_STOP;
>         if (!lazilyImportGlobalSlot(shape->slot))
>             RETURN_STOP_A("lazy import of global slot failed");
>+        pendingGlobalSlotToSet = shape->slot;
>         vp = &obj->getSlotRef(shape->slot);

Ewww. Is there any easy way we can check here whether a write is going to occur? Sometimes it's just a read. I think it's always the case that some TR method on the stack knows this, and it's a trivial, static yes or no.

Either add a bool (or enum) argument and lay the plumbing to get that bit to where it's needed; or if you leave it like this, add a comment explaining why this is safe. The proof is a little weird -- we can't more generally be sloppy and give pendingGlobalSlotToSet a slot when we don't actually expect an assignment, so this is safe only if the property has no getter. We're safe because we do not trace the oddball getter+slot case. (Actually doubly safe, because I don't think we have that oddball case in the engine or in the browser. But I'm not sure.)

In TraceRecorder::name:
>         if (!pcval.isSlot())
>             RETURN_STOP_A("PCE is not a slot");
>         slot = pcval.toSlot();
>     }
> 
>     if (!lazilyImportGlobalSlot(slot))
>         RETURN_STOP_A("lazy import of global slot failed");
> 
>+    pendingGlobalSlotToSet = slot;

Same here.

In jstracer.h:
>+    bool globalSetExpected(unsigned slot) {
>+        if (pendingGlobalSlotToSet != (int)slot && tracker.has(&globalObj->getSlotRef(slot)))
>+            return false;
>+        pendingGlobalSlotToSet = -1;
>+        return true;
>+    }

This method name is kind of meh, but I have no better suggestion.

I think it would be better here to say:

          if (pendingGlobalSlotToSet != (int)slot)
              return !tracker.has(&globalObj->getSlotRef(slot));
          pendingGlobalSlotToSet = -1;
          return true;

so as not to clear the expectation when a non-tracked slot is set.

Though, if TM::monitorRecording is going to set this to -1, rather than asserting that it is already -1, then we don't need to modify it here at all.

I wonder if you can avoid sprinkling pendingGlobalSlotToSet assignments throughout by hacking something into TR::set or setImpl. That would be a better hack, I think. Less sprawling. Ask David or Andreas if you're unsure; I'm kind of unsure too.

If that's not feasible, I really want some kind of assertion somewhere that our pendingGlobalSlotToSet prediction is correct-- that is, we set it everywhere we emit a write to a global, and nowhere else. I don't think I can feel comfortable that that's the case just by inspecting the code.

Clearing the r? request. If you address the above, I could r+ this as it stands.

Comment 16

[Luke Wagner [:luke]]

(In reply to comment #15)
> We still trace the opcodes that use record_SetPropHit with this patch, right?
> The interpreter calls record_SetPropHit *before* setting the slot. Yes? Just
> checking.

We don't abort a single time in the entire test suite except for the attached test case.

> Though, if TM::monitorRecording is going to set this to -1, rather than
> asserting that it is already -1, then we don't need to modify it here at all.

Setting it to -1 catches a second unexpected write to the global.  (Or, the second could be the expected write and the first was unexpected.)

> I wonder if you can avoid sprinkling pendingGlobalSlotToSet assignments
> throughout by hacking something into TR::set or setImpl.

Hah, duh, I don't know why I didn't think of that :)  We are even already checking for writes to globals there.

Thanks!

Comment 17

[Luke Wagner [:luke]]

Attachment: patch 3

This patch sets pendingGlobalSlotToSet in only one place (writeBack), which should address the comments.

Comment 18

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/38f985b0522c

Comment 19

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/38f985b0522c