Last Comment Bug 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"
: TM: (64-bit) "Assertion failed: LIR type error (start of writer pipeline): ar...
Status: RESOLVED FIXED
: [sg:critical] fixed-in-tracemonkey [c...
: assertion, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine
: Trunk
: All All
: P1 critical (vote)
: mozilla1.9.3a5
Assigned To: Brendan Eich [:brendan]
: general
:
:
: jsfunfuzz
  Show dependency treegraph
 
Reported: 2010-04-11 01:50 PDT by Gary Kwong [:nth10sd]
Modified: 2011-03-29 19:23 PDT (History)
13 users (show)
gary: in‑testsuite?
See Also:
Crash Signature:
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  final+
  ---
  .14+
  .14-fixed
  .17+
  .17-fixed


Attachments
TMFLAGS=readlir output on first testcase in comment 2 (5.29 KB, text/plain)
2010-04-23 21:22 PDT, Gary Kwong [:nth10sd]
no flags Details
TMFLAGS=readlir output on second testcase in comment 5 (9.57 KB, text/plain)
2010-04-23 21:23 PDT, Gary Kwong [:nth10sd]
no flags Details
LIR output (5.56 KB, text/plain)
2010-04-24 02:13 PDT, Nicholas Nethercote [:njn]
no flags Details
fix (831 bytes, patch)
2010-05-18 17:58 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
fix (851 bytes, patch)
2010-05-19 00:10 PDT, Brendan Eich [:brendan]
mrbkap: review+
Details | Diff | Splinter Review
191 patch (874 bytes, patch)
2011-01-20 15:53 PST, Jeff Walden (remove +bmo to email)
dveditz: approval1.9.1.17+
Details | Diff | Splinter Review
192 patch (874 bytes, patch)
2011-01-20 15:56 PST, Jeff Walden (remove +bmo to email)
dveditz: approval1.9.2.14+
Details | Diff | Splinter Review

Summon comment box

Description Gary Kwong [:nth10sd] 2010-04-11 01:50:13 PDT
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 [:nth10sd] 2010-04-11 01:51:19 PDT
And oh, this seems 64-bit only, on both Mac and Linux.
Comment 2 Gary Kwong [:nth10sd] 2010-04-19 03:56:04 PDT
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 [:nth10sd] 2010-04-21 22:47:57 PDT
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 2010-04-21 22:53:28 PDT
Kinda scary. I think bisecting might be pointing to a red herring.
Comment 5 Gary Kwong [:nth10sd] 2010-04-23 08:16:18 PDT
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 [:njn] 2010-04-23 15:12:29 PDT
(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 [:nth10sd] 2010-04-23 21:22:38 PDT
Created attachment 441239 [details]
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 [:nth10sd] 2010-04-23 21:23:59 PDT
Created attachment 441240 [details]
TMFLAGS=readlir output on second testcase in comment 5
Comment 9 Gary Kwong [:nth10sd] 2010-04-23 21:25:22 PDT
(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 [:njn] 2010-04-24 02:13:50 PDT
Created attachment 441261 [details]
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 11 Andreas Gal :gal 2010-04-26 19:23:48 PDT

*** This bug has been marked as a duplicate of bug 546611 ***
Comment 12 David Anderson [:dvander] 2010-04-26 19:24:10 PDT
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 [:njn] 2010-04-26 19:34:20 PDT
dvander, can you CC me on bug 546611?  Thanks.
Comment 14 Daniel Veditz 2010-05-09 17:16:31 PDT
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 [:nth10sd] 2010-05-09 22:49:51 PDT
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 2010-05-11 13:20:44 PDT
clearing the deprecated [sg:investigate] so it gets covered in the weekly mtg.
Comment 17 Johnny Stenback (:jst, jst@mozilla.com) 2010-05-18 13:12:10 PDT
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] 2010-05-18 13:26:10 PDT
Nanojit assertions suggest that we're generating code wrong. I'll look into the test case in comment #15.
Comment 19 Brendan Eich [:brendan] 2010-05-18 17:58:23 PDT
Created attachment 446110 [details] [review]
fix

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

/be
Comment 20 Brendan Eich [:brendan] 2010-05-18 18:03:15 PDT
Comment on attachment 446110 [details] [review]
fix

Grr, not right.

/be
Comment 21 David Anderson [:dvander] 2010-05-18 18:28:31 PDT
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] 2010-05-18 18:31:29 PDT
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] 2010-05-18 18:45:38 PDT
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] 2010-05-19 00:10:47 PDT
Created attachment 446168 [details] [review]
fix
Comment 25 Brendan Eich [:brendan] 2010-05-19 00:12:04 PDT
The indirect-eval-gets-Call-object bug is bug 566773.

/be
Comment 26 Brendan Eich [:brendan] 2010-05-19 16:26:25 PDT
http://hg.mozilla.org/tracemonkey/rev/0bed5abf1764

/be
Comment 28 Jeff Walden (remove +bmo to email) 2011-01-20 15:53:20 PST
Created attachment 505585 [details] [review]
191 patch

Port to branch was pretty pedestrian, nothing tricky about it.
Comment 29 Jeff Walden (remove +bmo to email) 2011-01-20 15:56:37 PST
Created attachment 505589 [details] [review]
192 patch

...likewise trivial.
Comment 30 Daniel Veditz 2011-01-21 10:21:29 PST
Comment on attachment 505585 [details] [review]
191 patch

Approved for 1.9.1.17, a=dveditz for release-drivers
Comment 31 Daniel Veditz 2011-01-21 10:22:05 PST
Comment on attachment 505589 [details] [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 33 Al Billings [:abillings] 2011-01-25 14:59:25 PST
Based on comment 15, what is the best way to verify this fix for 1.9.1 and 1.9.2?

Note You need to log in before you can comment on or make changes to this bug.