590553 - asm_ui2d on x86 generates illegal writes below ESP, caught by valgrind

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect, P1)

Description

[Tommy Reilly]

==87975== Invalid write of size 4
==87975==    at 0x1E65D5F: ???
==87975==    by 0x77D97: avmplus::BaseExecMgr::invoke_generic(avmplus::MethodEnv*, int, int*) (exec.cpp:619)
==87975==    by 0x78A2E: avmplus::BaseExecMgr::jitInvokerNext(avmplus::MethodEnv*, int, int*) (exec-jit.cpp:85)
==87975==    by 0x77362: avmplus::BaseExecMgr::verifyInvoke(avmplus::MethodEnv*, int, int*) (exec.cpp:240)
==87975==    by 0x795ED: avmplus::FunctionObject::call(int, int*) (MethodEnv-inlines.h:147)
==87975==    by 0xC5D1E: int avmplus::callprop_b<avmplus::Toplevel*>(avmplus::Toplevel*, int, avmplus::Multiname const*, int, int*, avmplus::VTable*, avmplus::Binding_*) (instr-inlines.h:133)
==87975==    by 0x7F73B: avmplus::interpBoxed(avmplus::MethodEnv*, int, int*) (Toplevel-inlines.h:47)
==87975==    by 0x7798C: avmplus::BaseExecMgr::invoke_interp_nocoerce(avmplus::MethodEnv*, int, int*) (exec.cpp:782)
==87975==    by 0x779CF: avmplus::BaseExecMgr::init_invoke_interp_nocoerce(avmplus::MethodEnv*, int, int*) (exec.cpp:193)
==87975==    by 0x77362: avmplus::BaseExecMgr::verifyInvoke(avmplus::MethodEnv*, int, int*) (exec.cpp:240)
==87975==    by 0x50F62: avmplus::AvmCore::callScriptEnvEntryPoint(avmplus::ScriptEnv*) (MethodEnv-inlines.h:141)
==87975==    by 0x51234: avmplus::AvmCore::handleActionPool(avmplus::PoolObject*, avmplus::Toplevel*, avmplus::CodeContext*) (AvmCore.cpp:705)
==87975==  Address 0xbfffefcc is just below the stack ptr.  To suppress, use: --workaround-gcc296-bugs=yes

Comment 1

[Tommy Reilly]

0x1ee0725   [begin]
    immi4 = immi 4
    immi6 = immi 0
0x1ee0725   xor eax,eax
    lti1 = lti argc, immi6/*0*/
    cmovi1 = cmovi lti1 ? immi6/*0*/ : argc
0x1ee0727   cmp ecx,0
0x1ee072a   cmovge eax,ecx
    restargcHelper1 = calli.all #restargcHelper ( immi6/*0*/ cmovi1 )
0x1ee072d   sub esp,8
0x1ee0730   push eax
0x1ee0731   push 0
0x1ee0733   call restargcHelper
0x1ee0738   add esp,16
    ui2d1 = ui2d restargcHelper1
0x1ee073b   mov -8(esp),eax
0x1ee073f   mov -4(esp),0

Comment 2

[Tommy Reilly]

valgrind complains about the last two instructions

Comment 3

[Tommy Reilly]

Attachment: valgrind ~/builds/tr-valgrind/release-debugger/shell/avmshell -Dnodebugger -Dverbose=jit as3/Definitions/Function/MultipleExtraArgFunction1.abc

Comment 4

[Edwin Smith]

The cause is in Assembler::asm_ui2d in Nativei386.cpp.  If the target register for LIR_ui2d is not an SSE register, then the generated code stores an int64 at ESP-8, the converts to double with FILDQ.  (your code sample would show this if you cut+pasted one more instruction :-))

My understanding of X86 is that this is legal.  not so?

Comment 5

[Edwin Smith]

Comment on attachment 474028 [details]
valgrind ~/builds/tr-valgrind/release-debugger/shell/avmshell -Dnodebugger -Dverbose=jit as3/Definitions/Function/MultipleExtraArgFunction1.abc


Here's the offending sequence in verbose output:

    ui2d1 = ui2d restargcHelper1
0x1edff2b   mov -8(esp),eax
0x1edff2f   mov -4(esp),0
0x1edff37   fildq -8(esp)

It is generated by asm_ui2d() in nanojit/Nativei386.cpp.

Comment 6

[Tommy Reilly]

edwin sez:
just as I suspected:
    pushl   %edx
    pushl   %eax
    fildll  (%esp)
    leal    8(%esp), %esp

Comment 7

[Edwin Smith]

This valgrind documentation implies we're doing the same illegal thing that gcc 2.96 did.  It would be nice to have a spec to point to that declares it illegal, but we probably need to change our JIT anyway.

http://valgrind.org/docs/manual/mc-manual.html#opt.workaround-gcc296-bugs

here's GCC output w/out SSE enabled:

    movl    8(%ebp), %eax   // eax = uint32 value
    movl    $0, %edx        // edx = 0
    pushl   %edx
    pushl   %eax
    fildll  (%esp)
    leal    8(%esp), %esp

our assembler needs to either leave 8 bytes of room above ESP for scrach, or emit code like GCC that protects the scratch area.  Allocating spill-space for the result value (the double result) would be a cheap way to get 8 bytes of protected scratch.

Comment 8

[Edwin Smith]

Simple AS3 test case:

function ui2d(u:uint):Number {
  return u
}
print(ui2d(1))

generates the offending code on x86 jit builds.

Comment 9

[Edwin Smith]

Attachment: Emit code similar to GCC output

Comment 10

[Tommy Reilly]

valgrind happy now
http://hg.mozilla.org/users/treilly_adobe.com/tr-valgrind/rev/b54d49e7b0fb

Comment 11

[Rick Reitmaier]

Not that I'm expecting it or that its all that important, but just curious if anyone noticed a difference in performance (aka vprof timing), between the two forms.

Comment 12

[Edwin Smith]

I need to run a round of benchmarks before this lands.  But I doubt it will matter.  the same number of loads and stores happen, the only new aspect is the mix of implicit and direct access to ESP, which might cause pipeline stalls.

Comment 13

[Edwin Smith]

This patch probably should target Salt and the next convenient FF 3.6.x release; in principal, it could silently corrupt the value of the LIR_ui2d instruction.  Not likely to result in a crash, but still very bad.  The only way this bug is benign is if every x86-based OS we all ship on, never writes to the region from ESP-8 to ESP in a signal handler.

Comment 14

[Nicholas Nethercote [inactive]]

Comment on attachment 474050 [details] [diff] [review]
Emit code similar to GCC output

So this only affects the non-SSE2 code?  That's good.

I'd like a short comment, something like "make sure we don't write past the end of the stack, see bug 590553".  r=me with that.

Comment 16

[Edwin Smith]

It also occurs on SSE2 machines that use x87 to return double values.  We encountered it on everyday mac machines.

Comment 17

[Nicholas Nethercote [inactive]]

(In reply to comment #16)
> It also occurs on SSE2 machines that use x87 to return double values.  We
> encountered it on everyday mac machines.

Oh right, then we should backport this.  Sayre, Gal, how do I flag a bug for backporting to 3.6.x -- is it "blocking1.9.2"?

Comment 18

[Edwin Smith]

Attachment: Core-2 performance run

Performance run with patch - no interesting outliers.

Comment 19

[Edwin Smith]

NJ: http://hg.mozilla.org/projects/nanojit-central/rev/482913608566

Comment 20

[Rick Reitmaier]

http://hg.mozilla.org/tamarin-redux/rev/b3bd24af7c85

Comment 21

[Edwin Smith]

Salt: p4 describe 717851

Comment 22

[Daniel Veditz [:dveditz]]

Blocking old branches 1.9.1/1.9.2

Shouldn't this also then block 2.0? Or will it magically get there via some later nanojit->mozilla-central landing?

Comment 23

[Daniel Veditz [:dveditz]]

note: blocking on the stable branches does not mean you can check in, patches always need approval (like "release candidate" stage on the trunk). I assume this simple patch will apply as-is, but please add the approval requests to the patch so we can be sure (or add a backported patch if necessary).

Comment 24

[Edwin Smith]

Leaving this open until it propagates everywhere it needs to.

Comment 25

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/27cb341980f2

Comment 26

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/27cb341980f2

Comment 27

[LegNeato]

Rob, can we get this landed on 1.9.2 and 1.9.1?

Comment 28

[LegNeato]

not sg:crit -> punt to next version.