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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

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)

Comment 1

[Christian Holler (:decoder)]

Attachment: Test case for shell

Test case

Comment 2

[Nicholas Nethercote [inactive]]

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.

Comment 3

[Brendan Eich [:brendan]]

So this is a regression from bug 613692 -- right?

/be

Comment 4

[Nicholas Nethercote [inactive]]

(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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove remnants of the assumption that inc() and incHelper() preserve type.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Nicholas Nethercote [inactive]]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Fixed those nits, and pushed as http://hg.mozilla.org/tracemonkey/rev/e10bd9b6cea0

Comment 12

[Daniel Veditz [:dveditz]]

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?

Comment 13

[Boris Zbarsky [:bzbarsky]]

I don't know offhand; njn or dvander might.

Comment 14

[Nicholas Nethercote [inactive]]

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?

Comment 15

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

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