Closed Bug 558633 Opened 10 years ago Closed 10 years ago

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"

Categories

(Core :: JavaScript Engine, defect, P1, critical)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.3a5
Tracking Status
blocking2.0 --- final+
blocking1.9.2 --- .14+
status1.9.2 --- .14-fixed
blocking1.9.1 --- .17+
status1.9.1 --- .17-fixed

People

(Reporter: gkw, Assigned: brendan)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [sg:critical] fixed-in-tracemonkey [critsmash:patch] [qa-examined-191] [qa-examined-192])

Attachments

(4 files, 3 obsolete files)

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).
And oh, this seems 64-bit only, on both Mac and Linux.
Summary: TM: "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" → TM: (64-bit) "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"
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)
    }
})()
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)
Kinda scary. I think bisecting might be pointing to a red herring.
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)
    }
}
(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.
> 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
Summary: TM: (64-bit) "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" → 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"
(In reply to comment #6)
> LIR type errors are really bad, this needs to be fixed.

Setting s-s first just to be safe.
Group: core-security
Attached file 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()?
Attachment #441239 - Attachment is obsolete: true
Attachment #441240 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 546611
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.
dvander, can you CC me on bug 546611?  Thanks.
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
Keywords: qawanted
Whiteboard: [sg:dupe 546611]
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
Status: RESOLVED → REOPENED
blocking2.0: --- → ?
Flags: in-testsuite?
Keywords: qawanted
Resolution: DUPLICATE → ---
Whiteboard: [sg:dupe 546611] → [sg:investigate]
clearing the deprecated [sg:investigate] so it gets covered in the weekly mtg.
Whiteboard: [sg:investigate]
Whiteboard: [sg:critical?][critsmash:investigating]
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?
Nanojit assertions suggest that we're generating code wrong. I'll look into the test case in comment #15.
Attached patch fix (obsolete) — Splinter Review
David, could you verify this fixes all tracing and JM bugs (if any)? Thanks,

/be
Assignee: general → brendan
Status: REOPENED → ASSIGNED
Attachment #446110 - Flags: review?(mrbkap)
Attachment #446110 - Attachment is obsolete: true
Attachment #446110 - Flags: review?(mrbkap)
Comment on attachment 446110 [details] [diff] [review]
fix

Grr, not right.

/be
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.
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
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!
No longer blocks: 547314
Whiteboard: [sg:critical?][critsmash:investigating] → [sg:critical]
Attached patch fixSplinter Review
Attachment #446168 - Flags: review?(mrbkap)
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.3a5
The indirect-eval-gets-Call-object bug is bug 566773.

/be
Attachment #446168 - Flags: review?(mrbkap) → review+
http://hg.mozilla.org/tracemonkey/rev/0bed5abf1764

/be
Whiteboard: [sg:critical] → [sg:critical] fixed-in-tracemonkey
blocking2.0: ? → final+
Whiteboard: [sg:critical] fixed-in-tracemonkey → [sg:critical] fixed-in-tracemonkey [critsmash:patch]
http://hg.mozilla.org/mozilla-central/rev/0bed5abf1764
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking1.9.1: ? → .17+
blocking1.9.2: ? → .14+
Attached patch 191 patchSplinter Review
Port to branch was pretty pedestrian, nothing tricky about it.
Attachment #505585 - Flags: approval1.9.2.14?
Attachment #505585 - Flags: approval1.9.2.14? → approval1.9.1.17?
Attached patch 192 patchSplinter Review
...likewise trivial.
Attachment #505589 - Flags: approval1.9.2.14?
Comment on attachment 505585 [details] [diff] [review]
191 patch

Approved for 1.9.1.17, a=dveditz for release-drivers
Attachment #505585 - Flags: approval1.9.1.17? → approval1.9.1.17+
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?
Attachment #505589 - Flags: approval1.9.2.14? → approval1.9.2.14+
Based on comment 15, what is the best way to verify this fix for 1.9.1 and 1.9.2?
Whiteboard: [sg:critical] fixed-in-tracemonkey [critsmash:patch] → [sg:critical] fixed-in-tracemonkey [critsmash:patch] [qa-examined-191] [qa-examined-192]
Group: core-security
Tracer has been long gone on trunk, marking verified.
Status: RESOLVED → VERIFIED
Filter on qa-project-auto-change:

Bug in removed tracer code, setting in-testsuite- flag.
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.