Closed Bug 561954 Opened 10 years ago Closed 9 years ago

TM: Make LeaveTraceIfGlobalObject abort recording

Categories

(Core :: JavaScript Engine, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- -
status2.0 --- wanted

People

(Reporter: dvander, Assigned: luke)

Details

(Whiteboard: [sg:nse] fixed-in-tracemonkey)

Attachments

(2 files, 4 obsolete files)

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.
Group: core-security
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.
Whiteboard: [sg:nse]
blocking2.0: --- → beta1+
blocking2.0: beta1+ → final+
Assignee: general → lw
Attached patch crashSplinter Review
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.
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?
(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.
Attached patch patch (obsolete) — Splinter Review
Aborts recording in js_NativeGet and js_DefineNativeProperty.
Attached patch patch (obsolete) — Splinter Review
Please be suspicious of my placement of AbortRecordingIfChangingGlobalSlotType.
Attachment #491078 - Attachment is obsolete: true
Attachment #491242 - Flags: review?(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.
Attachment #491242 - Flags: review?(jorendorff) → review+
Good point, much nicer in jstracer.h at the end.
http://hg.mozilla.org/tracemonkey/rev/31e0cd11f015
Whiteboard: [sg:nse] → [sg:nse] fixed-in-tracemonkey
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...
Whiteboard: [sg:nse] fixed-in-tracemonkey → [sg:nse]
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.
Attached patch patch 2 (obsolete) — Splinter Review
Kinda like this.
Attached patch patch 2 (obsolete) — Splinter Review
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.
Attachment #491913 - Attachment is obsolete: true
Attachment #491965 - Flags: review?(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.
Attachment #491965 - Flags: review?(jorendorff)
(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!
Attached patch patch 3Splinter Review
This patch sets pendingGlobalSlotToSet in only one place (writeBack), which should address the comments.
Attachment #491242 - Attachment is obsolete: true
Attachment #491965 - Attachment is obsolete: true
Attachment #495543 - Flags: review?(jorendorff)
Attachment #495543 - Flags: review?(jorendorff) → review+
blocking2.0: final+ → -
status2.0: --- → wanted
Whiteboard: [sg:nse] → [sg:nse] fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/38f985b0522c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.