779595 - IonMonkey: Crash on heap with invalid read

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on ionmonkey revision 21f15c00b5df (run with --ion -n):


var SECTION = "15.5.4.10-1";
function testInt8Array(L) {
    var f = new Int8Array(8);
}
for (var i = 0; i < 100000; ++i) {
    testInt8Array(SECTION,this);
}

Comment 1

[Christian Holler (:decoder)]

This could be the same as bug 779592, but I don't see any recursion involved here.

Crash info:

Program received signal SIGSEGV, Segmentation fault.
0x00429c48 in ?? ()
(gdb) bt
#0  0x00429c48 in ?? ()
#1  0xffffff87 in ?? ()
#2  0x00000000 in ?? ()
(gdb) x /i $pc
=> 0x429c48:    mov    0x14(%esi),%edi
(gdb) info reg esi edi
esi            0x0      0
edi            0xf770be60       -143606176

Comment 2

[Kannan Vijayan [:djvj]]

Playing with this on tip.. I get a different stack trace for the crash:

#0  0x00000001010f05f1 in ?? ()
#1  0x00000001003df9ff in js::mjit::EnterMethodJIT (cx=0x101115b50, fp=0x101200030, code=0x1010efe0a, 
    stackLimit=0x1015e0000, partial=true)
    at /Users/kvijayan/Checkouts/ionmonkey/js/src/methodjit/MethodJIT.cpp:1036
#2  0x00000001003dfe32 in CheckStackAndEnterMethodJIT (cx=0x101115b50, fp=0x101200030, code=0x1010efe0a, 
    partial=true) at /Users/kvijayan/Checkouts/ionmonkey/js/src/methodjit/MethodJIT.cpp:1094
#3  0x00000001003dfe84 in js::mjit::JaegerShotAtSafePoint (cx=0x101115b50, safePoint=0x1010efe0a, partial=true)
    at /Users/kvijayan/Checkouts/ionmonkey/js/src/methodjit/MethodJIT.cpp:1112
#4  0x000000010014f0b1 in js::Interpret (cx=0x101115b50, entryFrame=0x101200030, 
    interpMode=js::JSINTERP_NORMAL) at jsinterp.cpp:1489
#5  0x000000010014ca35 in js::RunScript (cx=0x101115b50, script=0x1021070e8, fp=0x101200030)
    at jsinterp.cpp:321
#6  0x00000001001643b1 in js::ExecuteKernel (cx=0x101115b50, script=
      {<JS::HandleBase<JSScript *>> = {<No data fields>}, ptr = 0x7fff5fbff538}, scopeChain=@0x102103060, 
    thisv=@0x7fff5fbff450, type=js::EXECUTE_GLOBAL, evalInFrame=0x0, result=0x0) at jsinterp.cpp:505
#7  0x000000010016464e in js::Execute (cx=0x101115b50, script=
      {<JS::HandleBase<JSScript *>> = {<No data fields>}, ptr = 0x7fff5fbff538}, scopeChainArg=@0x102103060, 
    rval=0x0) at jsinterp.cpp:542
#8  0x00000001000693cf in JS_ExecuteScript (cx=0x101115b50, objArg=0x102103060, scriptArg=0x1021070e8, 
    rval=0x0) at jsapi.cpp:5631
#9  0x000000010000b76f in Process (cx=0x101115b50, obj_=0x102103060, filename=0x7fff5fbffcb4 "/tmp/test.js", 
    forceTTY=false) at js.cpp:435
#10 0x0000000100009d82 in ProcessArgs (cx=0x101115b50, obj_=0x102103060, op=0x7fff5fbff9e8) at js.cpp:4852
#11 0x0000000100008e0e in Shell (cx=0x101115b50, op=0x7fff5fbff9e8, envp=0x7fff5fbffb30) at js.cpp:4893
#12 0x000000010000a72f in main (argc=4, argv=0x7fff5fbffb08, envp=0x7fff5fbffb30) at js.cpp:5097


There seems to be some weird code generation happening in the crash I am encountering.  The crash itself is happening at address 0x1010f05f1 (at the end of the following pasted disassembly block):

0x1010f05c4:	jmpq   *%rax
0x1010f05c6:	add    %al,(%rax)
0x1010f05c8:	movl   $0x2,0x90(%rbx)
0x1010f05d2:	mov    %rbx,0xb0(%rbx)
(gdb) 
0x1010f05d9:	mov    $0x1010efa5e,%r11
0x1010f05e3:	mov    %r11,0xb8(%rbx)
0x1010f05ea:	add    $0x90,%rbx
0x1010f05f1:	mov    0x28(%r12),%r15

(with %r12 == 0), leading to a bad deref of address 0x28.

After looking around nearby code, the only reference I could see that flowed into that code came from here (the jbe at the end of the block):


0x1010efdf8:	mov    $0x1003eb230,%r11
0x1010efe02:	callq  *%r11
0x1010efe05:	mov    0x38(%rsp),%rbx
0x1010efe0a:	jmpq   0x1010ef99c
0x1010efe0f:	add    %cl,-0x77(%rbp)
0x1010efe12:	out    %eax,$0x4d
0x1010efe14:	mov    0x28(%rdi),%esp
0x1010efe17:	mov    0xa0(%r12),%r12
0x1010efe1f:	mov    $0x1,%r11
0x1010efe29:	cmp    %r11,%r12
0x1010efe2c:	jbe    0x1010f05c8

The odd thing about this is that %r12 is getting loaded, compared with the constant "1", and then jumping to a few instructions before where the crash occurs.  Clearly this isn't the specific code that flowed into the crash since %r12 == 0 at the point of the crash.  Presumably there's some other location (which I can't find) which also jumps to 0x1010f05c8.  However, this particular seems to suggest that %r12 is not supposed to be a pointer, but is getting treated as such.

Comment 3

[Kannan Vijayan [:djvj]]

So turning off JM -> Ion stubs makes this crash go away.

Comment 4

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 5e461fb2250a).

Comment 5

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

Attachment: fix

The bug here is that |ic.funObjReg| could be clobbered before branching to the next out-of-line cache.

Comment 6

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

(Sorry for stealing this, it was on my queue to look into because it could have been related to bug 777788).

Comment 8

[Jan de Mooij [:jandem]]

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

Review of attachment 650727 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/ion/bug779595.js
@@ +1,5 @@
> +var SECTION = "15.5.4.10-1";
> +function testInt8Array(L) {
> +    var f = new Int8Array(8);
> +}
> +for (var i = 0; i < 12000; ++i) {

Nit: use count limit is 12400 I think + a bit more for OSR so I'd just use 13000.

::: js/src/methodjit/MonoIC.cpp
@@ +581,1 @@
>              masm.move(ic.funObjReg, funObjReg);

Just to be sure: if ic.funObjReg == Registers::ClobberInCall, it's okay to clobber the (original) ic.funObjReg, because we won't jump to another stub after this happens?

Comment 9

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

(In reply to Jan de Mooij (:jandem) from comment #8)
> ::: js/src/methodjit/MonoIC.cpp
> @@ +581,1 @@
> >              masm.move(ic.funObjReg, funObjReg);
> 
> Just to be sure: if ic.funObjReg == Registers::ClobberInCall, it's okay to
> clobber the (original) ic.funObjReg, because we won't jump to another stub
> after this happens?

Right, once we've clobbered the funObjReg we're already definitely going to call into Ion.

Comment 10

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

https://hg.mozilla.org/projects/ionmonkey/rev/cb2c22f6bb92

Comment 11

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 14

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij (:jandem) from comment #8)
>
> Nit: use count limit is 12400 I think + a bit more for OSR so I'd just use
> 13000.
> 

Sorry, not sure what I was thinking here, it's 10240 of course..