Closed Bug 617617 Opened 11 years ago Closed 11 years ago

LIR type error (start of writer pipeline): arg 1 of 'orq' is 'addd' which has type double (expected quad)

Categories

(Core :: JavaScript Engine, defect, P1)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: decoder, Assigned: bzbarsky)

References

Details

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

Attachments

(2 files)

The attached code causes an assertion (tested on trunk and jaegermonkey trunk) when executed in the js shell (tested on x86-64):

Assertion failure: LIR type error (start of writer pipeline): arg 1 of 'orq' is 'addd' which has type double (expected quad): 0 (./nanojit/LIR.cpp:3207)
Attached file Test case for shell
Test case
blocking2.0: --- → ?
Assignee: general → nnethercote
blocking2.0: ? → betaN+
Nb: don't specify -j when running the test -- it has |options('tracejit')| at the top, so -j will toggle the tracejit *off*.


The problematic LIR code is this:

00016:  23  incprop "x"
    ------------------------------ # JSOP_INCPROP
    guard_kshape2 = immi 1
    ldq2 = ldq.slots $global1[72]
    andq2 = andq ldq2, JSVAL_PAYLOAD_MASK/*0x7fffffffffff*/
    rshuq2 = rshuq ldq2, immi2/*47*/
    q2i2 = q2i rshuq2
    eqi3 = eqi q2i2, JSVAL_TYPE_TO_TAG(type)/*0x1fff5*/
    xf7: xf eqi3 -> exit=0x13a0270 pc=0x138f090 imacpc=(nil) sp+88 rp+8 BRANCH
    js_StringToNumber1 = calld.none #js_StringToNumber ( cx andq2 )
    immd1 = immd 1
    addd1 = addd js_StringToNumber1, immd1/*1*/
    std.sp sp[80] = addd1
    orq2 = orq addd1, shiftedTag/*0xfffa800000000000*/
    stq.slots $global1[72] = orq2

incHelper() is the culprit.  When incrementing a string, it changes |v_ins| to |call js_StringToNumber(cx_ins, v_ins)|, thus changing its type.  But it doesn't change |v|, so |v| is still a string.  So |v| and |v_ins| have inconsistent types.

Then stobj_set_slot() gets passed |v| and |v_ins|, and control ends up in
the 64-bit version of box_value_int(), which uses the type of |v| to determine what to do with |v_ins|, which leads to a type error.

bz, I'm reassigning to you -- I hope this is ok, this is your code and I'm not sure how to fix it.
Assignee: nnethercote → bzbarsky
Status: NEW → ASSIGNED
Summary: LIR type error assertion → LIR type error (start of writer pipeline): arg 1 of 'orq' is 'addd' which has type double (expected quad)
Group: core-security
So this is a regression from bug 613692 -- right?

/be
Blocks: 613692
(In reply to comment #3)
> So this is a regression from bug 613692 -- right?

I think bug 605858 is the root cause, but I haven't actually confirmed that.
This isn't actually a regression from bug 613692; that changed the v_ins that incHelper returns, but there are two codepaths that call incHelper.  On codepath one, via incName, v_ins is a stack variable that doesn't propagate out of incName.  On the other codepath, via inc(), v_ins is an inout param, and is set to v_after before returning.  So the real regression is indeed from bug 605858.
Blocks: 605858
No longer blocks: 613692
Duplicate of this bug: 620348
Comment on attachment 498804 [details] [diff] [review]
Remove remnants of the assumption that inc() and incHelper() preserve type.

I did verify that the nonminimal test is fixed too; the attached trace-test is based on code inspection of the obvious failing codepath, and asserts without this patch.
Attachment #498804 - Flags: review?(dvander)
Comment on attachment 498804 [details] [diff] [review]
Remove remnants of the assumption that inc() and incHelper() preserve type.

dvander is away until Jan 3, so I'll steal this review.  It looks good, I
have some nits/suggestions below that you might like to address.  Thanks for
fixing this!


> /*
>  * On exit, v_ins is the incremented unboxed value, and the appropriate value
>- * (pre- or post-increment as described by pre) is stacked.
>+ * (pre- or post-increment as described by pre) is stacked.  v_out is set to
>+ * the value corrsponding to v_ins.
>  */

Nit: s/corrsponding/corresponding.


>-    CHECK_STATUS_A(incHelper(v, v_ins, v_after, incr));
>-    LIns* v_result = pre ? v_after : v_ins;
>+    Value v_after;
>+    CHECK_STATUS_A(incHelper(v, v_ins, v_after, v_ins_after, incr));
>+    LIns* v_result = pre ? v_ins_after : v_ins;

Nit: maybe rename v_result as v_ins_result here, for consistency?


>+    /*
>+     * |v_out| is an out param whose value corresponds to the instruction the
>+     *  v_ins inout param gets set to.
>+     */
>+    JS_REQUIRES_STACK RecordingStatus inc(const Value &v, nanojit::LIns*& v_ins,
>+                                          Value &v_out, jsint incr,
>+                                          bool pre = true);
>+    /*
>+     * |v_after| is an out param whose value corresponds to the instruction the
>+     *  v_ins_after out param gets set to.
>+     */
>     JS_REQUIRES_STACK RecordingStatus incHelper(const Value &v, nanojit::LIns*& v_ins,
>-                                                  nanojit::LIns*& v_after, jsint incr);
>+                                                Value &v_after,
>+                                                nanojit::LIns*& v_ins_after,
>+                                                jsint incr);

Nit: The convention within jstracer.{cpp,h} appears to be that comments like
this go above the definition in jstracer.cpp, rather than above the
declaration in jstracer.h.
Attachment #498804 - Flags: review?(dvander) → review+
Fixed those nits, and pushed as http://hg.mozilla.org/tracemonkey/rev/e10bd9b6cea0
Whiteboard: fixed-in-tracemonkey
Whiteboard: fixed-in-tracemonkey → [sg:critical?] fixed-in-tracemonkey
Priority: -- → P1
Keywords: regression
Is this likely to be exploitable? If we have a String that we think is a Number will we just get wrong values? Or will it cause us to jump into arbitrary memory somehow?
I don't know offhand; njn or dvander might.
I don't think it's exploitable, because it causes us to treat a String as a Number, so we get a wrong value as Daniel says.  If we treated a Number as a String that would be more dangerous.

But this code has only appeared in beta releases, right?  In which case exploit potential doesn't matter that much, does it?
http://hg.mozilla.org/mozilla-central/rev/e10bd9b6cea0
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Group: core-security
Whiteboard: [sg:critical?] fixed-in-tracemonkey → [sg:nse] fixed-in-tracemonkey
You need to log in before you can comment on or make changes to this bug.