989166 - OdinMonkey: Invalid read and write of size 8, possibly in JIT code

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

(function(stdlib) {
    "use asm";
    var pow = stdlib.Math.pow
    function f(d0, i1) {
        d0 = +d0;
        i1 = i1 | 0
        var d2 = .0;
        var d3 = .0
        d2 = ((.0) % (+pow(d3, (.0))))
    }
    return f
})(this, {}, ArrayBuffer)()

shows an invalid read and write of size 8 with the js debug shell on m-c changeset c16f36021958 with the following command:

valgrind --vex-iropt-register-updates=allregs-at-mem-access --leak-check=full --smc-check=all-non-file ./js-opt-32-dm-vg-ts-linux-c16f36021958 testcase.js

s-s because this is an invalid read & write.

My configure flags are:

CC="gcc -m32" CXX="g++ -m32" AR=ar PKG_CONFIG_LIBDIR=/usr/lib/pkgconfig sh /home/gkwong/trees/mozilla-central/js/src/configure --target=i686-pc-linux --enable-optimize=-O1 --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --enable-valgrind --with-ccache --enable-threadsafe <other NSPR options>

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/a63e23e9b03b
user:        Benjamin Bouvier
date:        Thu Dec 12 20:23:29 2013 +0100
summary:     Bug 904918: Odin Float32 support; p=bbouvier,dougc r=luke,sstangl

Benjamin, is bug 904918 a likely regressor?

Comment 1

[Luke Wagner [:luke]]

Gary, is that that write to below the stack pointer?  (The store to -4($esp)?)

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Patch + test

(In reply to Sean Stangl [:sstangl] from comment #10)
> Comment on attachment 801939 [details] [diff] [review]
> Part 1 - AsmJS nodes MIR / LIR / Codegen
> 
> Review of attachment 801939 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +693,5 @@
> >          return;
> >  
> > +    if (mir->type() == MIRType_Float32) {
> > +        masm.reserveStack(sizeof(float));
> > +        masm.fstp32(Operand(esp, 0));
> 
> It occurs to me that we don't need to actually call reserveStack() and
> freeStack(), since we can just use the address at |Operand(esp,
> -(sizeof(float))|, which is like an implicit reservation. Likewise for the
> double case.

Still a good idea but V doesn't seem to really like it (and saying it explicitly: "Address 0xfe9d75f8 is just below the stack ptr.  To suppress, use: --workaround-gcc296-bugs=yes"). Should we either replace back the code to its previous form or just pass the flag to Valgrind when running the fuzzers?

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Luke Wagner [:luke] from comment #1)
> Gary, is that that write to below the stack pointer?  (The store to
> -4($esp)?)

Yup, this is the testcase that I personally showed to you and Julian just now (if that's what you are referring to), the disassembly was in the attachment but I'll reproduce here:

Valgrind backtrace of the first error:

(gdb) disas 0x4048010,0x4048050
Dump of assembler code from 0x4048010 to 0x4048050:
   0x04048010:  xorps  %xmm0,%xmm0
   0x04048013:  movsd  %xmm0,(%esp)
   0x04048018:  movsd  %xmm0,0x8(%esp)
   0x0404801e:  mov    $0x849e2e2,%eax
   0x04048023:  call   *%eax
=> 0x04048025:  fstpl  -0x8(%esp)
   0x04048029:  movsd  -0x8(%esp),%xmm0
   0x0404802f:  mov    $0x1,%eax
   0x04048034:  and    $0xfffffffe,%eax
   0x04048037:  mov    $0xfffffff2,%ecx
   0x0404803c:  mov    %cx,0x459aa10(%eax)
   0x04048043:  add    $0x1c,%esp
   0x04048046:  ret
   0x04048047:  hlt
   0x04048048:  hlt
   0x04048049:  hlt
   0x0404804a:  hlt
   0x0404804b:  hlt
   0x0404804c:  hlt
   0x0404804d:  hlt
   0x0404804e:  hlt
   0x0404804f:  hlt
End of assembler dump.


(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> Still a good idea but V doesn't seem to really like it (and saying it
> explicitly: "Address 0xfe9d75f8 is just below the stack ptr.  To suppress,
> use: --workaround-gcc296-bugs=yes"). Should we either replace back the code
> to its previous form or just pass the flag to Valgrind when running the
> fuzzers?

I was under the impression that using this flag wasn't desirable, perhaps Julian might be able to assist here?

Comment 4

[Julian Seward [:jseward]]

Just to be clear: reading and writing below %esp is disallowed by the
32 bit ELF ABI.  Don't do it.  Sooner or later that area is going to get
trashed by a signal delivery and the value you temp through that area
will be corrupted.

The 64 bit ELF x86 ABI does allow access to the 128 bytes below %rsp,
but the 32 bit ELF x86 ABI doesn't.

Comment 5

[Nicholas Nethercote [inactive]]

Historical context: Valgrind's flag is called --workaround-gcc296-bugs=yes because GCC 2.96 was buggy and generated accesses below the stack pointer. You shouldn't use it unless you are compiling with GCC 2.96... and you aren't :)

Comment 6

[Sean Stangl [:sstangl]]

Comment on attachment 8398291 [details] [diff] [review]
Patch + test

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

That review was misguided; I had indeed forgotten to consider signals. Thanks for catching it.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b682074d04a

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4b682074d04a

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8398291 [details] [diff] [review]
Patch + test

[Approval Request Comment]
Regression caused by (bug #): 904918
User impact if declined: stack corruption leading to crashes
Testing completed (on m-c, etc.): testing completed locally + m-i + m-c
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: n/a

Comment 10

[Marty Rosenberg [:mjrosenb]]

> That review was misguided; I had indeed forgotten to consider signals.
> Thanks for catching it.

AIUI, even if all of your signal handlers explicitly handle this case, the kernel can unmap everything below %esp any time it feels like it (e.g. context switches)  so while it will auto-fill that page the next time you ask for it, the data you wrote won't be there.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bf1134d72f5a
https://hg.mozilla.org/releases/mozilla-beta/rev/1eec473f0de0