Closed
Bug 970643
Opened 11 years ago
Closed 11 years ago
Valgrind does not understand OdinMonkey's guard page mechanism
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: jruderman, Assigned: jseward)
References
Details
(Whiteboard: [fuzzblocker])
Attachments
(4 files, 5 obsolete files)
1.90 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
2.67 KB,
text/plain
|
Details | |
23.94 KB,
text/plain
|
Details | |
5.40 KB,
patch
|
luke
:
review+
jseward
:
feedback+
lsblakk
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 2•11 years ago
|
||
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?
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(luke)
Comment 3•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
(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.
Flags: needinfo?(luke)
Comment 6•11 years ago
|
||
(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.
Assignee | ||
Comment 7•11 years ago
|
||
(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?
Assignee: nobody → jseward
Comment 8•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
(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.
Assignee | ||
Comment 13•11 years ago
|
||
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•11 years ago
|
||
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.
Attachment #8374919 -
Flags: review?(luke)
Comment 15•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #8374919 -
Flags: review?(luke) → review+
Assignee | ||
Comment 16•11 years ago
|
||
(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.
Assignee | ||
Comment 17•11 years ago
|
||
(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•11 years ago
|
||
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]
Flags: needinfo?(jruderman)
Whiteboard: [fuzzblocker] → [fuzzblocker][leave-open]
Assignee | ||
Comment 19•11 years ago
|
||
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.
Assignee | ||
Comment 20•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Attachment #8375585 -
Flags: feedback?(luke)
Comment 21•11 years ago
|
||
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).
Attachment #8375585 -
Flags: feedback?(luke) → feedback+
Assignee | ||
Comment 22•11 years ago
|
||
(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?
Flags: needinfo?(luke)
Comment 23•11 years ago
|
||
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.
Flags: needinfo?(luke)
Comment 24•11 years ago
|
||
(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.
Flags: needinfo?(jruderman)
Comment 26•11 years ago
|
||
(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•11 years ago
|
||
> 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•11 years ago
|
||
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.
Flags: needinfo?(nicolas.b.pierron)
Comment 29•11 years ago
|
||
(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.
Assignee | ||
Comment 30•11 years ago
|
||
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?
Attachment #8375591 -
Attachment is obsolete: true
Comment 31•11 years ago
|
||
> 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.
Flags: needinfo?(jseward)
Assignee | ||
Comment 32•11 years ago
|
||
> 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
Attachment #8375585 -
Attachment is obsolete: true
Flags: needinfo?(jseward)
Comment 33•11 years ago
|
||
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
Flags: needinfo?(jseward)
Comment 34•11 years ago
|
||
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.
Assignee | ||
Comment 35•11 years ago
|
||
(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_.
Flags: needinfo?(jseward)
Assignee | ||
Comment 36•11 years ago
|
||
Gary, does the patch-pair solve the jsfunfuzz noise problem for you?
Flags: needinfo?(gary)
Comment 37•11 years ago
|
||
(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)
Flags: needinfo?(gary)
Assignee | ||
Comment 38•11 years ago
|
||
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.
Attachment #8382966 -
Attachment is obsolete: true
Comment 39•11 years ago
|
||
Comment on attachment 8384564 [details] [diff] [review] bug970643-3-fx.cset Testing now.
Attachment #8384564 -
Flags: feedback?(gary)
Comment 40•11 years ago
|
||
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)")
Attachment #8382126 -
Attachment is obsolete: true
Attachment #8384564 -
Attachment is obsolete: true
Attachment #8384564 -
Flags: feedback?(gary)
Attachment #8385861 -
Flags: feedback?(jseward)
Updated•11 years ago
|
Attachment #8385861 -
Attachment description: fx-970643-c39.diff → fx-970643-c40.diff
Attachment #8385861 -
Attachment filename: fx-970643-c39.diff → fx-970643-c40.diff
Assignee | ||
Comment 41•11 years ago
|
||
(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.
Assignee | ||
Updated•11 years ago
|
Attachment #8385861 -
Flags: feedback?(jseward) → feedback+
Assignee | ||
Comment 43•11 years ago
|
||
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•11 years ago
|
||
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.
Flags: needinfo?(nicolas.b.pierron) → needinfo?(jseward)
Comment 45•11 years ago
|
||
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•11 years ago
|
||
> (no longer seems to be anymore false positives).
*(no longer seems to show false positives anymore)*
Assignee | ||
Updated•11 years ago
|
Attachment #8385861 -
Flags: review?(luke)
Flags: needinfo?(jseward)
Comment 47•11 years ago
|
||
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)
Attachment #8385861 -
Flags: review?(luke) → review+
Assignee | ||
Comment 48•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8510052b2313
Comment 50•11 years ago
|
||
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.
Whiteboard: [fuzzblocker][leave-open] → [fuzzblocker]
Comment 51•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8510052b2313
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 52•11 years ago
|
||
Julian, should we consider backporting the second landed patch (the first being nbp's) back to aurora? (if so, please nominate for aurora approval)
Flags: needinfo?(jseward)
Assignee | ||
Comment 53•11 years ago
|
||
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.
Attachment #8385861 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(jseward)
Comment 54•11 years ago
|
||
> 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•11 years ago
|
||
Comment on attachment 8385861 [details] [diff] [review] fx-970643-c40.diff What Nick said - please nominate the branch patch when ready.
Attachment #8385861 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
You need to log in
before you can comment on or make changes to this bug.
Description
•