As discussed in bug 838150, it makes sense to create a common interface for marking memory as noaccess/undefined/defined using Valgrind or AddressSanitizer. Quoting sewardj from that bug to describe how it's going to look like: > If I understand this correctly, I'd say that the Valgrind annotations > are a superset of the ASan ones, with the following mapping > > MAKE_MEM_NOACCESS --> poison_mem_region > MAKE_MEM_UNDEFINED --> unpoison_mem_region > MAKE_MEM_DEFINED --> unpoison_mem_region > > If you decide to make a unified interface, I'd suggest you use > the V 3-state scheme rather than the Asan 2-state scheme, and do > the mapping within the macros -- I assume that is what the proposal > was. And then use MAKE_MEM_NOACCESS and MAKE_MEM_UNDEFINED > respectively, in this function. I'm marking this s-s for now because once we have such a patch, it should get a bit of fuzzing at least from domfuzz because it could uncover additional issues if we add ASan support to some part of the code that ASan was previously unaware of.
A mfbt header with macros that wrap Valgrind/ASan functions sounds like a good idea. The three states and mapping that Julian suggested looks good to me. I assume you'll prefix the names with MOZ_, like MOZ_MEM_NOACCESS etc. The PresArena code could also use a common feature test name, something like: #if defined(MOZ_ASAN) || defined(MOZ_VALGRIND) #define MOZ_HAVE_MEM_CHECKS 1 #endif
Created attachment 712953 [details] [diff] [review] Patch Patch. Not tested on try yet, just trying to gather initial feedback whether this is the right thing :)
Not adding any new annotations in this bug, so we don't need to hide this.
Comment on attachment 712953 [details] [diff] [review] Patch Looks great!
Created attachment 713147 [details] [diff] [review] Patch v2 I missed a use of asan poisoning in debug-only code in LifoAlloc.cpp, but now it builds on Linux64 with ASan both debug and opt :)
Comment on attachment 713147 [details] [diff] [review] Patch v2 > layout/base/nsPresArena.cpp >-#if defined(MOZ_ASAN) >- ASAN_UNPOISON_MEMORY_REGION(result, list->mEntrySize); >-#elif defined(MOZ_VALGRIND) >- VALGRIND_MAKE_MEM_UNDEFINED(result, list->mEntrySize); >-#elif defined(DEBUG) >+ MOZ_MAKE_MEM_UNDEFINED(result, list->mEntrySize); >+#if defined(DEBUG) The last line should be (unless you intentionally wanted a change?): #if defined(DEBUG) && !defined(MOZ_HAVE_MEM_CHECKS) minor nit: > #include "mozilla/StandardInteger.h" >+#include "mozilla/MemoryChecking.h" Put MemoryChecking.h above for alphabetical order r=mats with those changes, except for js/* which needs review from a JS peer.
(In reply to Mats Palmgren [:mats] from comment #6) > Comment on attachment 713147 [details] [diff] [review] > Patch v2 > > The last line should be (unless you intentionally wanted a change?): > #if defined(DEBUG) && !defined(MOZ_HAVE_MEM_CHECKS) This is a good point. I did not intend to make this change (and in the order it is now it would break because we first poison and then run debug sanitizing code on the memory area). However, there is a good reason to enable it even with memchecks: If you create a Valgrind build, then you might not run it under Valgrind, but you also won't get the debug poisoning (which is a dangerous situation, we might miss a bug). We had a similar situation in the JS engine and decided to enable the poisoning unconditionally in debug builds. Of course, we must call MOZ_MAKE_MEM_UNDEFINED after this debug code then. Thoughts?
I did consider that when I wrote it, but I decided that the extra checks here for DEBUG+Valgrind builds wasn't worth it. I don't really care that much for this scenario though (Valgrind build and then NOT running under valgrind), so feel free to enable it for Valgrind builds if you want. I don't want it enabled in ASan builds though, because I want them to be as fast as possible and this check adds no value over what ASan already checks. I'd rather go the opposite route and disable frame poisoning writes too (in Free) for all MOZ_HAVE_MEM_CHECKS builds, since it's just a waste of time if you run under a mem-checker. IOW, I think we should declare that Valgrind builds are only for actually running under valgrind and nothing else. It's of much greater value to us to speed up ASan builds. (IIRC, valgrind has some interface to check if it's running or not, but that would just complicate the code further I think.)
> IOW, I think we should declare that Valgrind builds are only > for actually running under valgrind and nothing else. I disagree strongly. I always do debug builds of the JS shell with --enable-valgrind. And I always run the jit-tests on such builds with the --valgrind flag, but that only runs a handful of the tests under Valgrind; most of them run natively. So I think we should always do poisoning in debug builds, even if Valgrind/Asan are enabled.
(In reply to Christian Holler (:decoder) from comment #5) > Created attachment 713147 [details] [diff] [review] [diff] [review] > Patch v2 Looks plausible to me. W.r.t. the question should we do poisoning in debug builds, I have no opinion. For data race detection we may want to detect races where one thread accesses memory and another later frees it without coordination, which amounts to writing it at the free point, but race detectors can simulate that anyway. So there's no added benefit from doing it by hand.
I think I'm with Nicolas on this for two reasons: 1. I know more people that use Valgrind builds by default for debugging, and it's hard to impossible to declare Valgrind builds as only to be used with Valgrind. 2. Speed is irrelevant for ASan debug builds. Any testing that relies on speed should be done with opt builds, because ASan debug is very slow anyway and only meant for debugging and obtaining better traces. I'm going to reorder the patch accordingly.
Another possibility is to define MOZ_RUNNING_MEM_CHECKS to be: false in normal builds true in ASan builds (RUNNING_ON_VALGRIND > 0) in Valgrind builds and condition the poisoning on !MOZ_RUNNING_MEM_CHECKS.
Keep it simple, please! Just always poison on debug builds.
Created attachment 714057 [details] [diff] [review] Patch v3 Kept the poisoning unconditional in nsPresShell, but moved the call to memory sanitizer further down. Also requesting review from a JS peer now.
Reopened because the nsPresShell refactoring is not quite right in the patch. Of course the MOZ_MAKE_MEM_UNDEFINED line belongs *before* the poison checking block (as it was in the original patch), and not behind that. I guess I had poisoning in mind, not unpoisoning. I'll attach a patch that moves this to the original location, thereby fixing the startup crash on debug builds with asan.
Created attachment 714995 [details] [diff] [review] Follow-up fix Patch as described in last comment. Setting r+ because this is what mats originally reviewed already. Tested by cdiehl and confirmed to be working.
(In reply to Christian Holler (:decoder) from comment #18) > Created attachment 714995 [details] [diff] [review] > Follow-up fix > > Patch as described in last comment. Setting r+ because this is what mats > originally reviewed already. Tested by cdiehl and confirmed to be working. Hi, I am running TB under valgrind/memcheck and at the latest refresh of my source tree and re-run TB under memcheck, I suddenly encounter tons of "uninitialized memory" issues caused by this change. Are you sure if this is the right place to put MOZ_MAKE_MEM_UNDEFINED(result, list->mEntrySize); before the #if defined(DEBUG)/#endif ? Now, this patch obviously is in now comm-central, and I am running TB under valgrind. But since the MOZ_MAKE_MEM_UNDEFINED() is BEFORE the #if DEBUG block, and I think marks the area pointed by result pointer as uninitialized properly, memcheck cries out lout for EVERY invocation this routine (I think) about the reference to |val| in NS_ABORT_IF_VALSE() because the memory area is now obviously marked undefined, and just looking at |val| causes the uninitialized warning from valgrind. memcheck now prints out the following, an excerpt from the log : ==6240== Conditional jump or move depends on uninitialised value(s) ==6240== at 0x8F64D97: nsPresArena::AllocateBySize(unsigned int) (nsPresArena.cpp:342) [...] ==6240== Uninitialised value was created by a client request ==6240== at 0x8F64D69: nsPresArena::AllocateBySize(unsigned int) (nsPresArena.cpp:335) ==6240== by 0x9169BD5: nsRuleNode::ComputeVisibilityData(void*, nsRuleData const*, nsStyleContext*, nsRuleNode*, nsRuleNode::RuleDetail, bool) (nsIPresShell.h:274) I don't believe simply trying to suppress warning using the suppression file is the right solution since this error processing (even if it the printing warning is suppressed) is a major performance hit, I am afraid. Don't AddressSanitizer and other memory check tools have issues with this re-ordering of the position of memory check macro? memcheck certain has a problem with this ordering. (I checked the date of the patch and it was done toward the end of February?) I am using valgrind 3.9.0 (SVN) if it has any bearing on this problem. TIA
(In reply to ISHIKAWA, chiaki from comment #21) > Hi, I am running TB under valgrind/memcheck and > at the latest refresh of my source tree and re-run TB under memcheck, > I suddenly encounter tons of "uninitialized memory" issues caused by this > change. > > Are you sure if this is the right place to put > > MOZ_MAKE_MEM_UNDEFINED(result, list->mEntrySize); > > before the #if defined(DEBUG)/#endif ? No, it's not the right place, you are of course right :) The bug is already on file, see bug 852476 which also has the solution, just not as a patch yet. I will create a patch tomorrow. Sorry for the trouble! :)
(In reply to Christian Holler (:decoder) from comment #22) > > No, it's not the right place, you are of course right :) The bug is already > on file, see bug 852476 which also has the solution, just not as a patch > yet. I will create a patch tomorrow. Sorry for the trouble! :) Thank you, I looked at Bug 852476 and figure the intended processing. (Actually, I was wondering what the intention was. The old uncorrected code didn't look quite right :-) Now it is clear, and I will incorporate the suggested new change there. TIA