Closed Bug 590553 Opened 9 years ago Closed 9 years ago

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


(Core Graveyard :: Nanojit, defect, P1)



(blocking1.9.2 -, blocking1.9.1 -)

Tracking Status
blocking1.9.2 --- -
blocking1.9.1 --- -


(Reporter: treilly, Assigned: sayrer)



(Whiteboard: fixed-in-nanojit, fixed-in-tamarin, fixed-in-tracemonkey)


(3 files)

==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
Blocks: 509020
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
valgrind complains about the last two instructions
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 on attachment 474028 [details]
valgrind ~/builds/tr-valgrind/release-debugger/shell/avmshell -Dnodebugger -Dverbose=jit as3/Definitions/Function/

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.
edwin sez:
just as I suspected:
    pushl   %edx
    pushl   %eax
    fildll  (%esp)
    leal    8(%esp), %esp
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.

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.
Component: JIT Compiler (NanoJIT) → Nanojit
OS: Mac OS X → All
Product: Tamarin → Core
QA Contact: nanojit → nanojit
Summary: valgrind sez we're writing below the stack pointer in some acceptance tests → asm_ui2d on x86 generates illegal writes below ESP, caught by valgrind
Simple AS3 test case:

function ui2d(u:uint):Number {
  return u

generates the offending code on x86 jit builds.
Assignee: nobody → edwsmith
Attachment #474050 - Flags: review?(nnethercote)
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.
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.
Target Milestone: --- → flash10.2
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 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.
Attachment #474050 - Flags: review?(nnethercote) → review+
Duplicate of this bug: 520718
It also occurs on SSE2 machines that use x87 to return double values.  We encountered it on everyday mac machines.
(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"?
blocking1.9.2: --- → ?
blocking1.9.1: --- → ?
Attached file Core-2 performance run
Performance run with patch - no interesting outliers.
Priority: -- → P1
Whiteboard: fixed-in-nanojit → fixed-in-nanojit,fixed-in-tamarin
Target Milestone: flash10.2 → flash10.1.x-Salt
Salt: p4 describe 717851
Assignee: edwsmith → nobody
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?
blocking1.9.1: ? → .14+
blocking1.9.2: ? → .11+
blocking2.0: --- → ?
status2.0: --- → ?
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).
Closed: 9 years ago
Resolution: --- → FIXED
Leaving this open until it propagates everywhere it needs to.
Resolution: FIXED → ---
Whiteboard: fixed-in-nanojit,fixed-in-tamarin → fixed-in-nanojit, fixed-in-tamarin, fixed-in-tracemonkey
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Rob, can we get this landed on 1.9.2 and 1.9.1?
Assignee: nobody → sayrer
not sg:crit -> punt to next version.
blocking1.9.1: .14+ → needed
blocking1.9.2: .11+ → needed
blocking1.9.1: needed → .15+
blocking1.9.2: needed → .12+
blocking2.0: ? → ---
status2.0: ? → ---
blocking1.9.1: .16+ → -
blocking1.9.2: .13+ → -
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.