558633 - TM: (64-bit) "Assertion failed: LIR type error (start of writer pipeline): arg 1 of 'orq' is 'immi' which has type int32 (expected int64): 0 (../nanojit/LIR.cpp"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Console output:

$ cat 2interesting/1testcase.js 
__defineSetter__("x", eval)
__defineSetter__("a", /a/)
o = (function () {
    for (l in [0, 0, 0]) {
        for (var [, x] = /x/ in this) print(x)
    }
})()


$ jsfunfuzz-dbg-64-tm-40659-4932aaad4962/js-dbg-64-tm-darwin -j -e maxRunTime=5000 2interesting/1testcase.js 
undefined
undefined
undefined
undefined
a
undefined
undefined
undefined
Assertion failed: LIR type error (start of writer pipeline): arg 1 of 'orq' is 'imml' which has type int32 (expected int64): 0 (../nanojit/LIR.cpp:2634)
Abort trap


asserts js debug shell on TM tip with -j and with "-e maxRunTime=5000" at Assertion failed: LIR type error (start of writer pipeline): arg 1 of 'orq' is 'imml' which has type int32 (expected int64): 0 (../nanojit/LIR.cpp:2634)

I wonder why it requires "-e maxRunTime=5000", though the value "5000" can be any numerical value that I've tested.


autoBisect shows this is probably related to bug 547314:

The first bad revision is:
changeset:   38505:65eeef03da7c
user:        Andreas Gal
date:        Mon Feb 22 16:30:22 2010 -0800
summary:     Introduce ObjectOps for typeOf and make trace a mandatory ObjectOp (547314, r=brendan).

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

And oh, this seems 64-bit only, on both Mac and Linux.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Standalone testcase (only requires -j; thanks to Jesse for this tip):

maxRunTime=0
__defineSetter__("x", eval)
__defineSetter__("a", /a/)
o = (function () {
    for (l in [0, 0, 0]) {
        for (var [, x] = /x/ in this) print(x)
    }
})()

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

undefined
undefined
undefined
undefined
a
undefined
undefined
undefined
Assertion failed: LIR type error (start of writer pipeline): arg 1 of 'orq' is 'immi' which has type int32 (expected int64): 0 (../nanojit/LIR.cpp:2635)

Program received signal SIGABRT, Aborted.
0x00007ffff6ebfa75 in *__GI_raise (sig=<value optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
64	../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.
	in ../nptl/sysdeps/unix/sysv/linux/raise.c
(gdb) bt
#0  0x00007ffff6ebfa75 in *__GI_raise (sig=<value optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1  0x00007ffff6ec35c0 in *__GI_abort () at abort.c:92
#2  0x000000000059f8e2 in avmplus::AvmAssertFail () at ../nanojit/avmplus.cpp:65
#3  0x000000000059b086 in nanojit::ValidateWriter::typeCheckArgs (this=0x8c3598, op=nanojit::LIR_orq, nArgs=2, formals=0x7fffffffd690, args=0x7fffffffd680) at ../nanojit/LIR.cpp:2630
#4  0x000000000059bbe1 in nanojit::ValidateWriter::ins2 (this=0x8c3598, op=nanojit::LIR_orq, a=0x8bece0, b=0x8beb28) at ../nanojit/LIR.cpp:2960
#5  0x000000000055d450 in js::TraceRecorder::box_jsval (this=0x8bdcc0, v=8769092, v_ins=0x8bece0) at ../jstracer.cpp:9396
#6  0x0000000000564115 in js::TraceRecorder::callNative (this=0x8bdcc0, argc=1, mode=JSOP_CALL) at ../jstracer.cpp:10891
#7  0x0000000000564ba6 in js::TraceRecorder::functionCall (this=0x8bdcc0, argc=1, mode=JSOP_CALL) at ../jstracer.cpp:11034
#8  0x000000000056b9e2 in js::TraceRecorder::record_JSOP_CALL (this=0x8bdcc0) at ../jstracer.cpp:12576
#9  0x0000000000554c8f in js::TraceRecorder::monitorRecording (this=0x8bdcc0, op=JSOP_CALL) at ../jsopcode.tbl:179
#10 0x00000000005b17ed in js_Interpret (cx=0x8a53f0) at ../jsops.cpp:78
#11 0x0000000000485ca2 in js_Execute (cx=0x8a53f0, chain=0x7ffff6c02000, script=0x8af750, down=0x0, flags=0, result=0x0) at ../jsinterp.cpp:1103
#12 0x000000000042598b in JS_ExecuteScript (cx=0x8a53f0, obj=0x7ffff6c02000, script=0x8af750, rval=0x0) at ../jsapi.cpp:4827
#13 0x0000000000403970 in Process (cx=0x8a53f0, obj=0x7ffff6c02000, filename=0x7fffffffe620 "1testcase.js", forceTTY=0) at ../../shell/js.cpp:449
#14 0x0000000000404786 in ProcessArgs (cx=0x8a53f0, obj=0x7ffff6c02000, argv=0x7fffffffe320, argc=2) at ../../shell/js.cpp:863
#15 0x000000000040c783 in main (argc=2, argv=0x7fffffffe320, envp=0x7fffffffe338) at ../../shell/js.cpp:5038
(gdb)

Comment 4

[Andreas Gal :gal]

Kinda scary. I think bisecting might be pointing to a red herring.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Note that the original assertion was:

Assertion failed: LIR type error (start of writer pipeline): arg 1 of 'orq' is 'imml' which has type int32 (expected int64): 0 (../nanojit/LIR.cpp

and now it's morphed to:

Assertion failed: LIR type error (start of writer pipeline): arg 1 of 'orq' is 'immi' which has type int32 (expected int64): 0 (../nanojit/LIR.cpp:2635)

-> 'imml' changed to 'immi'

Here's another testcase that shows the latter assertion:

x = this
for (let z = 0; z < 3; z++) {
    for (var [c] = 3 in x) {
        print(c)
    }
}

Comment 6

[Nicholas Nethercote [inactive]]

(In reply to comment #5)
> 
> -> 'imml' changed to 'immi'

That opcode changed name very recently :)  Bug 555633.  So it hasn't really changed.

LIR type errors are really bad, this needs to be fixed.  It's Saturday morning in Melbourne so I don't have time to look in detail, but I suggest someone looks in TraceRecorder::box_jsval(), which has the only occurrences of LIR_pior in jstracer.cpp.  (Nb: LIR_pior ends up as LIR_orq on 64-bit, it's a bit confusing because we're halfway through the opcode renaming.)

Something that can help with diagnosing type errors is to change the NanoAssertMsgf() call in ValidateWriter::typeCheckArgs to a printf();  that way it doesn't stop on the error.  Combine that with TMFLAGS=readlir and it shouldn't be too hard to find the bad LIR instruction.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: TMFLAGS=readlir output on first testcase in comment 2

> Something that can help with diagnosing type errors is to change the
> NanoAssertMsgf() call in ValidateWriter::typeCheckArgs to a printf();  that way
> it doesn't stop on the error.  Combine that with TMFLAGS=readlir and it
> shouldn't be too hard to find the bad LIR instruction.

Area of code near where orq is located:

  31: js_String_getelem1 = callq. #js_String_getelem ( cx $stack8 1 )
  32: eqq4 = eqq js_String_getelem1, NULL
  33: xt2: xt eqq4 -> pc=0x1a4a7a4 imacpc=(nil) sp+56 rp+0 (GuardID=003)
  34: stq.s sp[40] = js_String_getelem1
  35: obj = immq 7F77:96E02000
  36: stq.s sp[48] = obj
  37: ATOM_TO_STRING(atom) = immq 0:85D120
  38: stq.s sp[56] = ATOM_TO_STRING(atom)
  39: JSVAL_STRING = immq 0:4
  40: orq2 = orq js_String_getelem1, JSVAL_STRING

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: TMFLAGS=readlir output on second testcase in comment 5

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #6)
> LIR type errors are really bad, this needs to be fixed.

Setting s-s first just to be safe.

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: LIR output

The problem is in instruction 66 (marked with '*') in this attachment:

* 66: orq1 = orq JSVAL_TO_SPECIAL(JSVAL_VOID), JSVAL_STRING

JSVAL_TO_SPECIAL(JSVAL_VOID) is a 32-bit immediate.  It must come from one of the INS_VOID() calls in jstracer.cpp.  

I'm pretty sure the 'orq' is generated by this line at the end of box_jsval():

        return lir->ins2(LIR_pior, v_ins, INS_CONSTWORD(JSVAL_STRING));

I don't know much beyond that, as I don't understand what this code is doing... maybe we need a pointer-sized alternative to INS_VOID()?

Comment 12

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

Explanation and whatnot coming in bug 546611, of which this is a dupe.

comment #5 and comment #10 show this is clearly and easily exploitable. Once you can tag an integer as an object/string, game over.

Comment 13

[Nicholas Nethercote [inactive]]

dvander, can you CC me on bug 546611?  Thanks.

Comment 14

[Daniel Veditz [:dveditz]]

qawanted:
   Is this really a dupe of bug 546611? Gary thought that one regressed Dec 11 due to bug 515749 and this one Feb 22 due to bug 547314. Gary: it would be great if you could verify that this bug is really fixed on trunk and 3.6.4

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Dan's right; this isn't a dupe (I tested using the testcase in comment 2). Switching back to [sg:investigate].

I couldn't get the comment 2 testcase to assert in 1.9.2 shell builds though. Nor could I get the comment 5 testcase to assert in any of the shells.

TM:

$ ~/Desktop/jsfunfuzz-dbg-64-tm-41833-d9ef93881da0/js-dbg-64-tm-linux -j
js> maxRunTime=0
0
js> __defineSetter__("x", eval)
js> __defineSetter__("a", /a/)
js> o = (function () {
    for (l in [0, 0, 0]) {
        for (var [, x] = /x/ in this) print(x)
    }
})()
undefined
undefined
undefined
undefined
a
undefined
undefined
undefined
Assertion failed: LIR type error (start of writer pipeline): arg 1 of 'orq' is 'immi' which has type int32 (expected int64): 0 (../nanojit/LIR.cpp:2640)
Aborted

m-c:

$ ~/Desktop/jsfunfuzz-dbg-64-mc-42011-b6726b0fa230/js-dbg-64-mc-linux -j
js> maxRunTime=0
0
js> __defineSetter__("x", eval)
js> __defineSetter__("a", /a/)
js> o = (function () {
    for (l in [0, 0, 0]) {
        for (var [, x] = /x/ in this) print(x)
    }
})()
undefined
undefined
undefined
undefined
a
undefined
undefined
undefined
Assertion failed: LIR type error (start of writer pipeline): arg 1 of 'orq' is 'immi' which has type int32 (expected int64): 0 (../nanojit/LIR.cpp:2640)
Aborted

Comment 16

[Daniel Veditz [:dveditz]]

clearing the deprecated [sg:investigate] so it gets covered in the weekly mtg.

Comment 17

[Johnny Stenback (:jst)]

This looks like an assert failure in nanojit, which would suggest that this is
not exploitable, unless this was seen in a debug build and asserts are no-ops
in release builds of nonojit. Can someone shed some light on whether this is
exploitable or not?

Comment 18

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

Nanojit assertions suggest that we're generating code wrong. I'll look into the test case in comment #15.

Comment 19

[Brendan Eich [:brendan]]

Attachment: fix

David, could you verify this fixes all tracing and JM bugs (if any)? Thanks,

/be

Comment 20

[Brendan Eich [:brendan]]

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

Grr, not right.

/be

Comment 21

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

Just for posterity, here's the explanation of what's going wrong.

 o = (function () {
    for (l in [0, 0, 0]) {
        for (var [, x] = /x/ in this) print(x)
    }

The destructuring assignment is not being recognized as a local slot, despite |var|. That's only half the bug. The real scary part (and why this asserts in the tracer) is that indirect eval calls js_GetCallObject for lightweight functions. This creates an intervening property on the call object, which the tracer does not see because of assumptions in BINDNAME.

Comment 22

[Brendan Eich [:brendan]]

The indirect eval is a global eval, but it gratuitously (left-over code) calls js_GetCallObject early on. David, Blake: can this be filed and fixed separately? It is not a security bug.

/be

Comment 23

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

Okay, so just to make sure I'm understanding this correctly. The call object imports locals, and this script has a local "x" defined. But the wrong opcode is emitted, breaking assumptions in BINDNAME. The interpreter binds to the local slot, the tracer to the global.

That's the security bug, so will file the the obj_eval one open. Thanks!

Comment 24

[Brendan Eich [:brendan]]

Attachment: fix

Comment 25

[Brendan Eich [:brendan]]

The indirect-eval-gets-Call-object bug is bug 566773.

/be

Comment 26

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/0bed5abf1764

/be

Comment 27

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/0bed5abf1764

Comment 28

[Jeff Walden [:Waldo]]

Attachment: 191 patch

Port to branch was pretty pedestrian, nothing tricky about it.

Comment 29

[Jeff Walden [:Waldo]]

Attachment: 192 patch

...likewise trivial.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 505585 [details] [diff] [review]
191 patch

Approved for 1.9.1.17, a=dveditz for release-drivers

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 505589 [details] [diff] [review]
192 patch

Approved for 1.9.2.14, a=dveditz for release-drivers

Needs to land by 13:00 PST, can you do that today?

Comment 32

[Jeff Walden [:Waldo]]

Aye aye, cap'n!

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6e7c740fdcb5
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7a28315a5aec

Comment 33

[Al Billings [:abillings - ex-MoCo]]

Based on comment 15, what is the best way to verify this fix for 1.9.1 and 1.9.2?

Comment 34

[Christian Holler (:decoder)]

Tracer has been long gone on trunk, marking verified.

Comment 35

[Christian Holler (:decoder)]

Filter on qa-project-auto-change:

Bug in removed tracer code, setting in-testsuite- flag.