620662 - TM: Assertion failure: peer != fragment, at ../jstracer.cpp:5015

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Test

Attached test case triggers this assert:

$ ./js -j test.js
Assertion failure: peer != fragment, at ../jstracer.cpp:5015

Comment 1

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

TraceRecorder::attemptTreeCall() says:

    // Refresh the import type map so the tracker can reimport values after the
    // call with their correct types. The inner tree must not change the type of
    // any variable in a frame above the current one (i.e., upvars).

This invariant is being broken in a bizarre way. Inner trees technically cannot mutate on-stack, on-trace callobj vars, but they can if the callobj is not on trace.

In this case, the inner tree mutates the upvar during recording, and thereafter the typemap information in the recorder is wrong. Wrong typemaps are a security concern, so flipping the bit.

Luke, think we could just abort recording the inner tree when mutating the on-stack call var?

Comment 2

[Luke Wagner [:luke]]

(In reply to comment #1)
> Luke, think we could just abort recording the inner tree when mutating the
> on-stack call var?

Do you mean abort in setCallProp case when callobj->getPrivate() && !frameIfInRange(callobj) ?  I think that would work and I am all for removing unnecessary tracer complexity.  However, last time I tried to remove a class of upvar usage, it ended up biting and this case is definitely more common than the upvar case.

So I was thinking "why don't we have this problem with the other classes of aliased slots: globals and locals.  The answer is: the importTypeMap is updated after the ExecuteTree call in attemptTreeCall.  So I think a better solution (that would also simplify the code) is to just move the DetermineTypesVisitor call from attemptTreeCall() to emitTreeCall().  (Btw, that second patch in bug 620637 simplifies this area by removing the JSVAL_TYPE_UNINITIALIZED gunk, so you might want to review that first.)

Comment 3

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

Okay, thanks, re-importing more frames was going to be suggestion #2 - I think we even used to do it until the most complex upvar code got removed.

Comment 4

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

Attachment: fix

It turns out we can't do that because we don't know how to interpret integer-speculated values that were written back as doubles. After talking with Luke, we decided to just abort (the nested tree will fall off trace anyway).

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 501477 [details] [diff] [review]
fix

Two nits: /* */ and perhaps inclusion of a sentence like "As documented in attemptTreeCall, when recording an inner tree call, the recorder assumes the inner tree does not mutate the tracked upvars."

Comment 6

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

http://hg.mozilla.org/tracemonkey/rev/f7a47c9fab0c with nits

Comment 9

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

http://hg.mozilla.org/mozilla-central/rev/f7a47c9fab0c

Comment 10

[Daniel Veditz [:dveditz]]

Did this turn out to be a real security bug? Neither this nor the earlier bug 620232 duped to this one were originally marked security bugs by the reporters. bug 620232 is still not marked as a security bug, different (less severe) bug that just happens to be fixed by the same patch?

Comment 11

[chris hofmann]

cdleary, dvander, luke,  any thoughts on comment 10?

Comment 12

[Luke Wagner [:luke]]

This can cause wrongly-typed reads of stack values, so a security bug.