532689 - Crash on args-range.js closure test with jit on in 64-bit builds (free memory read in 32-bit builds)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

dvander forwarded me dmandelin's closure tests, and I tried running them locally.  args-range.js consistently crashes for me with jit on (64-bit linux, debug and opt shells).  It passes fine on 32-bit Mac (debug and opt shells).

File is attached.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Log

OK.  Here's disas around the crash site:

(gdb) frame
#0  0x00007ffff1558ed1 in ?? ()
(gdb) disas 0x00007ffff1558ed1-32 0x00007ffff1558ed1+32
Dump of assembler code from 0x7ffff1558eb1 to 0x7ffff1558ef1:
0x00007ffff1558eb1:     jne    0x7ffff1548e6c
0x00007ffff1558eb7:     cmp    %rcx,%rcx
0x00007ffff1558eba:     jne    0x7ffff1548e91
0x00007ffff1558ec0:     mov    $0x86822a,%edx
0x00007ffff1558ec5:     imul   $0x1,%r14d,%ecx
0x00007ffff1558ecc:     mov    %ecx,%ecx
0x00007ffff1558ece:     add    %rcx,%rdx
0x00007ffff1558ed1:     movzbl 0x0(%rdx),%ecx
0x00007ffff1558ed5:     cmp    $0x1,%ecx
0x00007ffff1558ed8:     jne    0x7ffff1548eb6
0x00007ffff1558ede:     cmp    $0x1,%r14d
0x00007ffff1558ee2:     jae    0x7ffff1548edb
0x00007ffff1558ee8:     mov    %r13,%rcx
0x00007ffff1558eeb:     add    $0xffffffffffffffe0,%rcx
0x00007ffff1558eef:     imul   $0x8,%r14d,%edx

(gdb) info reg rdx
rdx            0x100868225      4303782437
(gdb) p * 0x100868225
Cannot access memory at address 0x100868225

So we're trying to read into rcx/ecx memory from the address pointed to by rdx... and can't. The relevant part of the TMFLAGS=full trace looks like:

      ldcb1 = ldcb qiadd2[0]
  7ffff1558ed1 movzxb rcx, 0(rdx)

and the relevant LIR chunk for that part is:

  62: typemap + 2 = quad #0:86822A /* 4.35526e-317 */
  63: sizeof(JSTraceType) = int 1
  64: mul1 = mul ld1, sizeof(JSTraceType)
  65: u2q1 = u2q mul1
  66: qiadd2 = qiadd typemap + 2, u2q1
  67: ldcb1 = ldcb qiadd2[0]
  68: guard(type-stable upvar) = eq ldcb1, sizeof(JSTraceType)

The whole log is attached.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So the relevant part of the assembly-generation seems to be:

      typemap + 2 = quad #0:86822A /* 4.35526e-317 */
  7ffff1558ec0 movl edx, 8815146
                            RR rax(PCVAL_TO_OBJECT(pcval)) rdx(typemap + 2) rbx(state) rdi(cx) r12(ld2) r13(sp) r14(ld1) r15(ldq3)
                            AR 4+(r15) 12+(r14) 20+(r13) 28+(r12) 36+(state) 44+(rbx)
      mul1 = mul ld1, sizeof(JSTraceType)
  7ffff1558ec5 imuli ecx, r14d, 1
                            RR rax(PCVAL_TO_OBJECT(pcval)) rcx(mul1) rdx(typemap + 2) rbx(state) rdi(cx) r12(ld2) r13(sp) r14(ld1) r15(ldq3)
                            AR 4+(r15) 12+(r14) 20+(r13) 28+(r12) 36+(state) 44+(rbx)
      u2q1 = u2q mul1
  7ffff1558ecc movl ecx, ecx
                            RR rax(PCVAL_TO_OBJECT(pcval)) rcx(u2q1) rdx(typemap + 2) rbx(state) rdi(cx) r12(ld2) r13(sp) r14(ld1) r15(ldq3)
                            AR 4+(r15) 12+(r14) 20+(r13) 28+(r12) 36+(state) 44+(rbx)
      qiadd2 = qiadd typemap + 2, u2q1
  7ffff1558ece addq rdx, rcx
                            RR rax(PCVAL_TO_OBJECT(pcval)) rdx(qiadd2) rbx(state) rdi(cx) r12(ld2) r13(sp) r14(ld1) r15(ldq3)
                            AR 4+(r15) 12+(r14) 20+(r13) 28+(r12) 36+(state) 44+(rbx)
      ldcb1 = ldcb qiadd2[0]
  7ffff1558ed1 movzxb rcx, 0(rdx)

Note that to compute the address to load from we add rcx to what's already in rdx and store the result in rdx.  Now I have:

(gdb) info reg rcx
rcx            0xfffffffb       4294967291
(gdb) p *(0x100868225-0xfffffffb)
$2 = 65537

So looks to me like rdx was pointing to about the right place, until we added rcx to it.  Note that 0xfffffffb is uint32(-5).

Comment 3

[Boris Zbarsky [:bzbarsky]]

So on 32-bit, I bet we do this addition, and that's equivalent to subtracting 5 from the typemap + 2.  That at least lands us in memory we know about.  On 64-bit, we end up 4 gigs away from the typemap, in lala-land.

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, so what we do here is apparently load in the typemap+2.  Then multiply ld1 by sizeof(JSTraceType) (which is 1; I'm a little sad that we didn't optimize this away).  Then u2q the result (which I'm again a little sad didn't get optimized away).  Then add to our typemap thing, etc.

The point being that ld1 is -5.  And indeed:

(gdb) info reg r14
r14            0xfffffffb       4294967291

So we really are trying to read the typemap at index -5.  On 32-bit we likely read garbage and fail the type guard.  On 64-bit, bad things happen.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Looks like this code is part of a guard:

    About to try emitting guard code for SideExit=0x868238 exitType=BRANCH
    typemap + 2 = quad #0:86822A /* 4.35526e-317 */
    sizeof(JSTraceType) = int 1
    mul1 = mul ld1, sizeof(JSTraceType)
    u2q1 = u2q mul1
    qiadd2 = qiadd typemap + 2, u2q1
    ldcb1 = ldcb qiadd2[0]
    guard(type-stable upvar) = eq ldcb1, sizeof(JSTraceType)
    xf4: xf guard(type-stable upvar) -> pc=0x869c08 imacpc=(nil) sp+32 rp+0 (GuardID=005)

We emit a "type-stable upvar" guard in two cases: record_JSOP_GETELEM for the arguments class, and upvar().  The latter guards on the type of an insCall return, so that's not this one.  The former looks like this:

                    LIns* typep_ins = lir->ins2(LIR_piadd, typemap_ins,
                                                lir->ins_u2p(lir->ins2(LIR_mul,
                                                                       idx_ins,
                                                                       INS_CONST(sizeof(JSTraceType)))));
                    LIns* type_ins = lir->insLoad(LIR_ldcb, typep_ins, 0);
                    guard(true,
                          addName(lir->ins2(LIR_eq, type_ins, lir->insImm(type)),
                                  "guard(type-stable upvar)"),
                          BRANCH_EXIT);

so that's our guard.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Aha!  So her's the relevant C++:

                // the native stack area. The guard on js_ArgumentClass above ensures the up-to-date
                // value has been written back to the native stack area.
                idx_ins = makeNumberInt32(idx_ins);
                if (int_idx >= 0 && int_idx < afp->argc) {

where before that we had:

   LIns* idx_ins = get(&idx);

We need to not only check that >= and < during record, but guard it on trace as well, no?  Guarding >= 0 is simple.  How do we guard on afp->argc?

Comment 7

[Robert Sayre]

why does this need to block?

Comment 8

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

(In reply to comment #7)
It's an arbitrary memory read, if I'm seeing this correctly.

(In reply to comment #6)
Nice find. I think you can just INS_CONST(afp->argc). Since the frame is in range, the argc is fixed on trace.

                if (int_idx >= 0 && int_idx < afp->argc) {
....
                } else {
                    guard(false, lir->ins2(LIR_ult, idx_ins, INS_CONST(afp->argc)),
                          snapshot(BRANCH_EXIT));
                    v_ins = INS_VOID();

That guard probably needs to be hoisted above the |if|.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

64 bit builds aren't supported for mozilla1.9.2, so if this is really 64-bit only, it shouldn't block.

Comment 10

[Boris Zbarsky [:bzbarsky]]

The crash is easy to reproduce on 64-bit builds with the given testcase (because we reference memory outside our mapped address range).

On 32-bit builds, we read memory we _do_ have mapped, but not the memory we meant to read, and the memory we read is entirely under the control of the script (it can read one byte from anywhere in memory).  The best-case scenario is that we then fail the typecheck and bail off trace, losing nothing other than performance.  If not, then the script is allowed to read a semi-arbitrary 4-byte value out of memory (has to be offset from our native stack by the same amount (number of jsvals) that the byte read was offset from our typemap (number of bytes).  Depending on what else happens after that we might crash, or the script might just give incorrect answers or ... something.  Given all the jumping to function pointers (class ops, etc) that the js engine does, giving script a chance to treat random memory it might have filled with payload as JSObjects gives me the willies.

Come to think of it, on 64-bit builds we could have similar behavior if the value used were closer to -2^31 and we actually had 2 gigs of ram mapped.

All of which is to say that this should generally be a safe crash on 64-bit builds, more or less, but on 32-bit builds all bets are off.  Resummarizing accordingly.

I'm not sure we should block, but I do think we should fix ASAP; in 1.9.2.1 if not in 1.9.2.  I'll try to find time to patch if no one else gets there first.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

My feeling is that we can wait for 3.6.1/1.9.2.1 for this.

Comment 12

[David Mandelin [:dmandelin]]

Attachment: Patch

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 421725 [details] [diff] [review]
Patch

>+++ b/js/src/jstracer.cpp
>+                    guard(true,
>+                          addName(lir->ins2(LIR_gt, idx_ins, INS_CONST(0)),
>+                                  "guard(upvar index >= 0)"),
>+                          BRANCH_EXIT);

That should be using LIR_ge, no?

>+                    guard(true,
>+                          addName(lir->ins2(LIR_le, idx_ins, INS_CONST(afp->argc)),
>+                                  "guard(upvar index in range)"),
>+                          BRANCH_EXIT);

And this should be LIR_lt.

Please add tests, especially for the latter?

Why do we not need snapshot() calls around those BRANCH_EXITs?

Comment 15

[Nicholas Nethercote [inactive]]

(In reply to comment #14)
> 
> Why do we not need snapshot() calls around those BRANCH_EXITs?

TraceRecorder::guard() is overloaded -- one of the versions does the snapshot() for you.

Comment 16

[David Mandelin [:dmandelin]]

Attachment: Patch 2 (fix range checks and add more tests)

Gah. I blame my bad range checks on the cold I seem to be getting. Anyway, args-range.js was the test case, since this is really a regression bug. But I added some more tests to try to test more corners, which is definitely a good thing.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 421750 [details] [diff] [review]
Patch 2 (fix range checks and add more tests)

Looks good.  So are we running your closure tests automatically now?

Comment 18

[David Mandelin [:dmandelin]]

(In reply to comment #17)
> (From update of attachment 421750 [details] [diff] [review])
> Looks good.  So are we running your closure tests automatically now?

I think so. They are part of trace-tests now, which is run by 'make check'.

Comment 19

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/tracemonkey/rev/5c06d8cc50b0

Comment 20

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/5c06d8cc50b0