970643 - Valgrind does not understand OdinMonkey's guard page mechanism

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

As we discovered in bug 913876, OdinMonkey's segfault-and-recovery mechanism for bounds checks confuses Valgrind. jsfunfuzz is hitting this often, and it is making it difficult to find real bugs (such as bug 969133).

A) Use --no-asmjs whenever fuzzing with Valgrind :(

B) Ignore all crashes in (all types of) JIT code when fuzzing with Valgrind :(

C) Teach Valgrind to ignore segfaults that are caught by the program -- perhaps as an option, perhaps by default. (bug 913876 comment 31)

D) Add a SpiderMonkey option to catch bounds errors with a different mechanism.

Note that "use ASan instead of Valgrind" is not an option when it comes to JIT testing, because ASan cannot instrument code generated by SpiderMonkey's JIT.

Comment 1

[Luke Wagner [:luke]]

Note, this isn't just used by OdinMonkey, but also IonMonkey (bug 864220).

Comment 2

[Julian Seward [:jseward]]

We should do a variant of (C).  In principle it should not be hard to
do, although I would prefer not to use the scheme mooted in bug 913876
comment 31 on the grounds that it's (a) hard to implement cleanly
given V's internal architecture and (b) it sounds like it could hide
real errors in other situations.

Since SpiderMonkey already uses hinting for valgrind as gated by
#ifdef MOZ_VALGRIND, my preferred scheme is to add hinting as to the
address ranges for which we expect to get segfaults:

  #ifdef MOZ_VALGRIND
    VALGRIND_DISABLE_ADDR_ERROR_REPORTING_IN_RANGE(a, len)
  #endif

  // later

  #ifdef MOZ_VALGRIND
    VALGRIND_ENABLE_ADDR_ERROR_REPORTING_IN_RANGE(a, len)
  #endif

Verbose but you get the idea.  The new macros would have to be
implemented.

* how many ranges and of what size do you expect to exist,
  approximately?

* are we expecting the faulting insns to be loads, stores, or
  both?

Comment 3

[Nicolas B. Pierron [:nbp]]

From what I understood this was not that valgrind did not understood the error reporting mechanism, but more that it did not expected to segfault while simulating some instructions.  Valgrind is emulating the instructions with its virtual registers and it is not dumping all the states to actual registers, which explains why this error disappear when we ask Valgrind to spill all registers as expected when running the program.

Could we have assembly patterns to precisely emulate any instructions between 2 macros?  Such as Valgrind is signal-safe within these sections?

(In reply to Julian Seward from comment #2)
> * how many ranges and of what size do you expect to exist,
>   approximately?

Basically, anything which is reading/writing the IonCode, which mostly concerns ICs (which are patching the code) and the CompactBufferReader read functions (which reads meta-data).

> * are we expecting the faulting insns to be loads, stores, or
>   both?

Both and also any other executed insns, as we are doing an mprotect on the execuatble code.

Comment 4

[Nicholas Nethercote [inactive]]

Jesse, are the fuzzers invoking Valgrind with --vex-iropt-register-updates=allregs-at-mem-access? I found that caused some random crashes to go away when I enabled it on the Valgrind-on-TBPL job.

Comment 5

[Luke Wagner [:luke]]

(In reply to Julian Seward from comment #2)
I don't see how executing handlers if they aren't the default disposition hides real errors.  You could imagine a fault during the handler, but this tends to lead to infinite recursion and eventually a crash.  Alternatively, Valgrind could consider any fault during handler to be a crash.

> * how many ranges and of what size do you expect to exist,
>   approximately?

There could be a few hundred ranges due to the Ion use of the SIGSEGV handler.

> * are we expecting the faulting insns to be loads, stores, or
>   both?

Load, store, execute.

Comment 6

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

(In reply to Nicholas Nethercote [:njn] from comment #4)
> Jesse, are the fuzzers invoking Valgrind with
> --vex-iropt-register-updates=allregs-at-mem-access? I found that caused some
> random crashes to go away when I enabled it on the Valgrind-on-TBPL job.

Yes, we have that flag.

Comment 7

[Julian Seward [:jseward]]

(In reply to Luke Wagner [:luke] from comment #5)
> > * are we expecting the faulting insns to be loads, stores, or
> >   both?
> 
> Load, store, execute.

I (approximately) get how the load/store faulting is used.  But how is
execute faulting used?  Why do we generate code and place it on a page
that lacks execute permission?

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Julian Seward from comment #7)
> (In reply to Luke Wagner [:luke] from comment #5)
> > > * are we expecting the faulting insns to be loads, stores, or
> > >   both?
> > 
> > Load, store, execute.
> 
> I (approximately) get how the load/store faulting is used.  But how is
> execute faulting used?  Why do we generate code and place it on a page
> that lacks execute permission?

(this reply the summary of an IRC discusssion I had with Julian)

We do generate code on executable pages first.  The reason why we might have execution fault is caused by the fact that we replaced a boolean check on for loops by an mprotect.

When we hit try to execute the protected memory, we go to the signal handler which unprotect[1] the memory if it is in the range of allocated memory.  After that, we patch the loop back-edge to jump to a location where we can interrupt the normal execution to do something else.

We have a "timeout" function in the JS shell which gives us the ability to use the interruption callback (which use the above mechanism), and I managed to reproduce a similar signature as we saw before with the following test case ran under valgrind:

  function f() {
    // interrupt any JS code in 2 seconds, and starts f after.
    timeout(2, f);
    var obj;
    // Do an infinite loop which produces garbage.
    while(true)
      obj = {a:{b:{c:{d:{}}}}};
  };
  f();

valgrind --smc-check=all-non-file ./dist/bin/js above-file.js
…
Script runs for too long, terminating.
Script runs for too long, terminating.
Script runs for too long, terminating.
Script runs for too long, terminating.
==11881== Invalid read of size 1
==11881==    at 0x68229E: js::jit::CompactBufferReader::readByte() (CompactBuffer.h:57)
==11881==    by 0x860F35: js::jit::CompactBufferReader::readFixedUint32_t() (CompactBuffer.h:60)
==11881==    by 0x8698FB: RelocationIterator::RelocationIterator(js::jit::CompactBufferReader&) (Assembler-x64.cpp:221)
==11881==    by 0x85E3B9: js::jit::Assembler::TraceJumpRelocations(JSTracer*, js::jit::JitCode*, js::jit::CompactBufferReader&) (Assembler-x64.cpp:258)
==11881==    by 0x70534B: js::jit::JitCode::trace(JSTracer*) (Ion.cpp:650)
==11881==    by 0x583294: js::gc::MarkChildren(JSTracer*, js::jit::JitCode*) (Marking.cpp:1183)
==11881==    by 0x583A85: js::GCMarker::processMarkStackOther(unsigned long, unsigned long) (Marking.cpp:1354)
==11881==    by 0x5D1564: js::GCMarker::processMarkStackTop(js::SliceBudget&) (Marking.cpp:1392)
==11881==    by 0x583B64: js::GCMarker::drainMarkStack(js::SliceBudget&) (Marking.cpp:1493)
==11881==    by 0x903852: DrainMarkStack(JSRuntime*, js::SliceBudget&, js::gcstats::Phase) (jsgc.cpp:4067)
==11881==    by 0x9056E1: IncrementalCollectSlice(JSRuntime*, long, JS::gcreason::Reason, js::JSGCInvocationKind) (jsgc.cpp:4627)
==11881==    by 0x905D27: GCCycle(JSRuntime*, bool, long, js::JSGCInvocationKind, JS::gcreason::Reason) (jsgc.cpp:4791)
==11881==  Address 0x0 is not stack'd, malloc'd or (recently) free'd

By the way, I wonder if the above signature is not a real bug, where the GC can be executed even if the memory is still protected, or is being protected on an other thread.

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/Ion.cpp#332

Comment 9

[Luke Wagner [:luke]]

As far as I know, it's not a real bug; it is expected that the Ion-fault can happen at any time.  The handler tries to be pretty careful in the handler not to assume too much about what is happening outside.

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Luke Wagner [:luke] from comment #9)
> As far as I know, it's not a real bug; it is expected that the Ion-fault can
> happen at any time.  The handler tries to be pretty careful in the handler
> not to assume too much about what is happening outside.

This stack corresponds to the moment where we are reading the meta-data which are stored after/within the code to mark all GC things.  So if the code is protected against reads/writes (and not only executions), then we would also have a problem in the GC.

Comment 11

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #10)
> This stack corresponds to the moment where we are reading the meta-data
> which are stored after/within the code to mark all GC things.  So if the
> code is protected against reads/writes (and not only executions), then we
> would also have a problem in the GC.

The signal handler use si_addr, which is the address of the instruction, and not the read/written address.

I think we should change the mask to only remove the EXEC flag.

http://dxr.mozilla.org/mozilla-central/source/js/src/assembler/jit/ExecutableAllocatorPosix.cpp#109

Comment 12

[Luke Wagner [:luke]]

(In reply to Nicolas B. Pierron [:nbp] from comment #11)
> The signal handler use si_addr, which is the address of the instruction, and
> not the read/written address.

No, 'pc' (from the CONTEXT) is the faulting pc; si_addr is the address being accessed.

> I think we should change the mask to only remove the EXEC flag.

Sure, we could do that.  It's not necessary though.

Comment 13

[Julian Seward [:jseward]]

I am trying but failing to reproduce this, thusly:

cd js/src

autoconf-2.13

mkdir build-release

cd build-release

../configure --enable-valgrind --disable-jemalloc --enable-threadsafe \
  --disable-intl-api --without-intl-api --enable-debug=-ggdb3 \
  --enable-debug-symbols --disable-elf-hack --disable-optimize \
  --enable-warnings-as-errors --enable-stdcxx-compat \
  --enable-profiling --enable-ctypes

make -j2

vTRUNK --smc-check=all-non-file --vex-iropt-register-updates=allregs-at-mem-access \
   ./js/src/shell/js -e \
  'function f() { timeout(2, f); var obj; while(true) obj = {a:{b:{c:{d:{}}}}}; }; f();'

gives

==24691== Memcheck, a memory error detector
==24691== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==24691== Using Valgrind-3.10.0.SVN and LibVEX; rerun with -h for copyright info
==24691== Command: ./js/src/shell/js -e function\ f()\ {\ timeout(2,\ f);\ var\ obj;\ while(true)\ obj\ =\ {a:{b:{c:{d:{}}}}};\ };\ f();
==24691== 
Script runs for too long, terminating.
-e:1:15 InternalError: too much recursion
==24691== 
==24691== HEAP SUMMARY:
==24691==     in use at exit: 0 bytes in 0 blocks
==24691==   total heap usage: 2,690 allocs, 2,690 frees, 2,953,553 bytes allocated
==24691== 
==24691== All heap blocks were freed -- no leaks are possible
==24691== 
==24691== For counts of detected and suppressed errors, rerun with: -v
==24691== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 1 from 1)

repeatedly; no segfault afaics.  Can you give me some better STR ?

Comment 14

[Nicolas B. Pierron [:nbp]]

Attachment: Only toggle execution permissions on JIT code.

I am checking it right now under valgrind, and so far I have not been able to reproduce the issue that I reported above.

This could be an issue on how valgrind initialize the siginfo, especially the si_addr field.

Comment 15

[Nicolas B. Pierron [:nbp]]

Sorry, I forgot to mention that I was not running with --vex-iropt-register-updates=allregs-at-mem-access
The above test does not fail if I add this additional flag, or if I use the previous patch.

Comment 16

[Julian Seward [:jseward]]

(In reply to Gary Kwong [:gkw] [:nth10sd] catching up on email/bugmail from comment #6)
> (In reply to Nicholas Nethercote [:njn] from comment #4)
> > Jesse, are the fuzzers invoking Valgrind with
> > --vex-iropt-register-updates=allregs-at-mem-access? I found that caused some
> > random crashes to go away when I enabled it on the Valgrind-on-TBPL job.
> 
> Yes, we have that flag.

To be clear: --vex-iropt-register-updates=allregs-at-mem-access is
necessary to make Valgrind's "baseline" simulation work properly in
the presence of such segfaultery.  By "work properly" I mean that
the Valgrind run actually stays alive and doesn't crash (itself or
Firefox).  

That flag has effectively become mandatory for running OdinMonkey
generated code and is not part of the issue here.

The issue here is how to stop Memcheck from complaining about the
associated invalid memory access, since that generates a lot of noise
in the fuzzing results and AIUI makes them more or less useless.

Comment 17

[Julian Seward [:jseward]]

(In reply to Julian Seward from comment #13)
> repeatedly; no segfault afaics.  Can you give me some better STR ?

I can reproduce with the testcase in bug 913876 comment 0.  Good.

Comment 18

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d75a799e1b1f

Jesse, are you still seeing a lot of valgrind issue? I do not know if this patch might help knowing that I cannot reproduce any issue when I using --vex-iropt-register-updates=allregs-at-mem-access .

I will let you close this bug it this patch fix the problem. [marking as leave-open]

Comment 19

[Julian Seward [:jseward]]

Attachment: bug970643-1-fx.diff

Proposed OdinMonkey-side fix for x86_64-linux, at least.  I think I
have the ifdeffery set up so that (1) it only has any effect on
--enable-valgrind builds, and (2) it can be built safely against both
"old" and "new" versions of memcheck.h, but will only have effect when
built against a new version of memcheck.h and run on a matching
Valgrind version.

Comment 20

[Julian Seward [:jseward]]

Attachment: bug970643-1-val.diff

Valgrind-side fixes needed to support by-address-range error report
enabling/disabling.  Proof-of-concept patch only.

Together with the OdinMonkey-side patch in comment 19, this removes
the report displayed when running the test case of comment 0 of bug
913876.

These patches address only the problem of hiding reports resulting
from reads and writes in the relevant areas.  I haven't yet looked
into V's behaviour when trying to execute code out of an invalid area.

Comment 21

[Luke Wagner [:luke]]

Comment on attachment 8375585 [details] [diff] [review]
bug970643-1-fx.diff

Looks good.  Again, this isn't the only source of safe SIGSEGV -- the other is when we mprotect executable code -- but that can also be disabled by the fuzzers by setting the env var JS_DISABLE_SLOW_SCRIPT_SIGNALS.  Unfortunately, that could hide actual bugs in the operation-callback logic (using the 'timeout' shell function).

Comment 22

[Julian Seward [:jseward]]

(In reply to Luke Wagner [:luke] from comment #21)
> Looks good.  Again, this isn't the only source of safe SIGSEGV -- the other
> is when we mprotect executable code -- but that can also be disabled by the
> fuzzers by setting the env var JS_DISABLE_SLOW_SCRIPT_SIGNALS. 

I plan to look at that as soon as I can get my hands on a reliable
repro case.  At least in principle it would be possible to get V to
honour mprotects on code areas.  I should point out that this might be
expensive in as much as removing execute permissions for a page will
cause V to throw away all instrumented code it has made from that
page.  Remaking instrumented code is expensive, so this isn't
something you want to be doing more than a few times per second.  Is
that within your expected performance parameters?

Comment 23

[Luke Wagner [:luke]]

The mprotect itself is expensive, so it should not happen frequently in the browser and, iirc, it will only happen very infrequently (when requested, which isn't often) in the shell.

Comment 24

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

(In reply to Nicolas B. Pierron [:nbp] from comment #18)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/d75a799e1b1f
> 
> Jesse, are you still seeing a lot of valgrind issue?

We'll be testing this again when Julian lands his patch. I originally saw enough false positives to discuss with Jesse and together we reported this bug.

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d75a799e1b1f

Comment 26

[Nicolas B. Pierron [:nbp]]

(In reply to Gary Kwong [:gkw] [:nth10sd] catching up on email/bugmail from comment #24)
> (In reply to Nicolas B. Pierron [:nbp] from comment #18)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/d75a799e1b1f
> > 
> > Jesse, are you still seeing a lot of valgrind issue?
> 
> We'll be testing this again when Julian lands his patch. I originally saw
> enough false positives to discuss with Jesse and together we reported this
> bug.

I would prefer if we can test again, before we start disabling Valgrind checks.  I do not want to realize by accident, that we missed tons of critical errors because we inhibit valgrind error reports.

Comment 27

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

> I would prefer if we can test again, before we start disabling Valgrind
> checks.  I do not want to realize by accident, that we missed tons of
> critical errors because we inhibit valgrind error reports.

Alright, I've started another Valgrind run on m-c tip right now.

Comment 28

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

Attachment: valgrind log of invalid read

g = (function(stdlib, foreign, heap) {
    "use asm";
    var Float64ArrayView = new stdlib.Float64Array(heap)
    var Uint8ArrayView = new stdlib.Uint8Array(heap)
    function f(i0) {
        i0 = i0 | 0;
        Float64ArrayView[Uint8ArrayView[i0] >> 3] = 0.0
    }
    return f;
})(this, {}, new ArrayBuffer(4096))
g(080000);

causes this invalid read using "valgrind --vex-iropt-register-updates=allregs-at-mem-access --smc-check=all-non-file ./js-opt-64-dm-vg-ts-er-linux-6687d299c464 --ion-parallel-compile=off --ion-eager testcase.js"

Tested on m-c rev 6687d299c464, which has nbp's patch.

nbp: So to answer your question, nope, these issues still occur.

Comment 29

[Luke Wagner [:luke]]

(In reply to Wes Kocher (:KWierso) from comment #25)
So Nicolas, this patch likely breaks our emulators since I think they are not faithfully checking execute permissions when emulating (see bug 975182 comment 11).  I think the reason that bug 975182 caught this and this bug didn't is because asm.js actually has tests for this.

Comment 30

[Julian Seward [:jseward]]

Attachment: bug970643-2-val.diff

An implementation of the Valgrind side of the fix that is
scalable (to hundreds of address ranges) and tested.  Suitable
for landing.  The Fx side of it (8375585: bug970643-1-fx.diff)
remains unchanged.

Jesse, Gary, can you test the two patches together, to see if
they solve the jsfunfuzz noise problem?

Comment 31

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

> The Fx side of it (8375585: bug970643-1-fx.diff)
> remains unchanged.

This patch needs to be rebased:

$ patch -p1 --dry-run < ~/Desktop/fx-970643-c21.diff
patching file js/src/vm/TypedArrayObject.cpp
Hunk #2 FAILED at 540.
Hunk #3 FAILED at 576.
2 out of 3 hunks FAILED -- saving rejects to file js/src/vm/TypedArrayObject.cpp.rej

Tested on m-c rev bb9edb4d5144.

Comment 32

[Julian Seward [:jseward]]

Attachment: bug970643-2-fx.cset

> This patch needs to be rebased:
Ah yes.  The context got moved entirely to a different file.

Rebased version herewith attached.

One thing you might want to be aware of is that running Valgrind
with -v shows you output as below, which is useful for checking
that the ignore-range hints are getting passed from SM to V.  If
you don't see those, you can be pretty sure something isn't
working right.

Warning: set address range perms: large range [0x39e08000, 0x139e09000) (noaccess)
memcheck: modify_ignore_ranges: add 0x39E0A000 0x139E08FFF
memcheck:   now have 3 ranges:
memcheck:      [0]  0000000000000000-0000000039e09fff  NotIgnored
memcheck:      [1]  0000000039e0a000-0000000139e08fff  ClientReq
memcheck:      [2]  0000000139e09000-ffffffffffffffff  NotIgnored

Warning: set address range perms: large range [0x39e08000, 0x139e09000) (noaccess)
memcheck: modify_ignore_ranges: del 0x39E08000 0x139E08FFF
memcheck:   now have 1 ranges:
memcheck:      [0]  0000000000000000-ffffffffffffffff  NotIgnored

Comment 33

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

Attachment: "Conditional jump or move depends on uninitialised value(s)" error stack

Tested on rev 4cfb6c61b137, with the 8382126: bug970643-2-val.diff and 8382966: bug970643-2-fx.cset patches.

valgrind -v --track-origins=yes --vex-iropt-register-updates=allregs-at-mem-access --leak-check=full --smc-check=all-non-file ./js --no-ti --ion-eager testcase.js

Comment 34

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

Oh, seems like I left out the testcase.

for (var i = 0; i < 4; ++i) {}
x = Array.buildPar(9, function() {});
y = x.filterPar(function() {
    return i
});
Array.prototype.every.call(y, (function() {
    "use asm";
    function f() {}
    return f
}))

Let me know if this is a separate issue altogether.

Comment 35

[Julian Seward [:jseward]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #34)
> Let me know if this is a separate issue altogether.

From having peered a bit in AsmJSModule.{h,cpp}, it seems to me that
this is unrelated.  Looks to me like there's a bug in that
AsmJSModule::AsmJSModule does not initialise
AsmJSModule::codeIsProtected_.

Comment 36

[Julian Seward [:jseward]]

Gary, does the patch-pair solve the jsfunfuzz noise problem for you?

Comment 37

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

(In reply to Julian Seward from comment #36)
> Gary, does the patch-pair solve the jsfunfuzz noise problem for you?

For the record, I don't think so. (Julian and I were liaising over IRC just minutes ago, I sent him some testcases, and he's trying to reproduce)

Comment 38

[Julian Seward [:jseward]]

Attachment: bug970643-3-fx.cset

Revised IM-side patch.  The previous patch only handled VM protections
done by ArrayBufferObject.cpp.  It appears though that SharedArrayObject.cpp
uses the same mechanism (with almost identical code).  This patch
covers both uses.  Are there any other places where this range setting
is done?

This patch fixes the problems with the testcases that Gary mentioned
in comment 37.

Comment 39

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

Comment on attachment 8384564 [details] [diff] [review]
bug970643-3-fx.cset

Testing now.

Comment 40

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

Attachment: fx-970643-c40.diff

bug970643-3-fx.cset didn't apply on m-c rev e5b09585215f which is tip now, so I rebased it. Will test this one.

Julian, please check that the rebasing makes sense. (in particular the part around "if defined(MOZ_VALGRIND) && defined(VALGRIND_DISABLE_ADDR_ERROR_REPORTING_IN_RANGE)")

Comment 41

[Julian Seward [:jseward]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #40)
> Julian, please check that the rebasing makes sense.

Yes, the rebasing looks OK to me.

Comment 42

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

I sent Julian some testcases found from testing the patch in comment 40.

Comment 43

[Julian Seward [:jseward]]

Here's an easy way to tell whether or not the relevant client requests
have been compiled into SM (or libxul for that matter).

VALGRIND_DISABLE_ADDR_ERROR_REPORTING_IN_RANGE and
VALGRIND_ENABLE_ADDR_ERROR_REPORTING_IN_RANGE, which are used by IM to
tell V about the ranges, generate respectively in the machine code,
the following magic constants that do not appear anywhere else:
0x4d43000e and 0x4d43000d.  Hence you can check easily if an
executable has them by using objdump and grep:

objdump -d js/src/shell/js | grep 0x4d43000e
  6fbfbb:        48 c7 45 a0 0e 00 43         movq   $0x4d43000e,-0x60(%rbp)
  76ff82:        48 c7 45 b0 0e 00 43         movq   $0x4d43000e,-0x50(%rbp)
objdump -d js/src/shell/js | grep 0x4d43000d
  6fc0c2:        48 c7 45 c0 0d 00 43         movq   $0x4d43000d,-0x40(%rbp)
  770052:        48 c7 45 c0 0d 00 43         movq   $0x4d43000d,-0x40(%rbp)

This is what I would expect to see for a SM build patched with the
comment 38/40 patch.  In that patch, each request appears twice, which
is in accordance with the grep output above.

On ARM, such constants are pieced together over multiple instructions
or hauled out of constant pools, so the above trick probably won't
work.  Should be ok for x86 and x86_64 though.

Comment 44

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

Preliminary results show that there no longer seems to be this issue anymore, with the 2 patches.

I'll need to test more to be sure, but still, this is a vast improvement, so please go ahead and land this to move forward.

Comment 45

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

After another overnight run, it is still clean (no longer seems to be anymore false positives).

Sidenote: I ran into a fuzzing harness bug in comment 42 - it was telling me the binary was patched when it really wasn't, so thanks Julian for coming up with comment 43 to see if it was indeed patched.

Comment 46

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

> (no longer seems to be anymore false positives).

*(no longer seems to show false positives anymore)*

Comment 47

[Luke Wagner [:luke]]

Comment on attachment 8385861 [details] [diff] [review]
fx-970643-c40.diff

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

::: js/src/vm/ArrayBufferObject.cpp
@@ +560,5 @@
>          VirtualFree(p, 0, MEM_RELEASE);
>          return false;
>      }
>  # else
> +    size_t valid_length = AsmJSPageSize + buffer->byteLength();

validLength (ditto for SharedArrayObject.cpp)

@@ +567,5 @@
>          return false;
>      }
> +#   if defined(MOZ_VALGRIND) && defined(VALGRIND_DISABLE_ADDR_ERROR_REPORTING_IN_RANGE)
> +    // Tell Valgrind/Memcheck to not report accesses in the inaccessible region.
> +    if (valid_length < AsmJSMappedSize) {

We have assertions elsewhere that valdLength is always < AsmJSMappedSize, so you can remove this test. (ditto for SharedArrayObject.cpp)

Comment 48

[Julian Seward [:jseward]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8510052b2313

Comment 49

[Julian Seward [:jseward]]

Valgrind components committed as revision 13884.

Comment 50

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

I think those are the remaining patches waiting to be landed, and I presume this can be resolved after the next merge from inbound to central.

Comment 51

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8510052b2313

Comment 52

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

Julian, should we consider backporting the second landed patch (the first being nbp's) back to aurora? (if so, please nominate for aurora approval)

Comment 53

[Julian Seward [:jseward]]

Comment on attachment 8385861 [details] [diff] [review]
fx-970643-c40.diff

[Approval Request Comment]

Bug caused by (feature/regressing bug #): 
  Not sure of the bug number(s), but basically the introduction of
  segfault-based bounds checks for arrays in OdinMonkey

User impact if declined: 
  None directly, but indirectly -- it makes fuzzing the JS engine
  using Valgrind difficult

Testing completed (on m-c, etc.): 
  Looked ok on try.  Hasn't been backed out in a week or two.

Risk to taking this patch (and alternatives if risky): 
  Minimal.  Will need to rebase, I suspect, considering it had
  to be rebased a couple of times during development on m-c.
  But is a near-trivial patch; should be no big deal.

String or IDL/UUID changes made by this patch:
  None.

Comment 54

[Nicholas Nethercote [inactive]]

> Risk to taking this patch (and alternatives if risky): 
>   Minimal.  Will need to rebase, I suspect, considering it had
>   to be rebased a couple of times during development on m-c.
>   But is a near-trivial patch; should be no big deal.

In that case, you should do so now and re-request approval on the updated patch. Aurora patches are landed by sheriffs, so they need to be in a landable state.

Comment 55

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8385861 [details] [diff] [review]
fx-970643-c40.diff

What Nick said - please nominate the branch patch when ready.