838557 - Create and use a common interface for ASan/Valgrind memory handling functions

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Mats Palmgren (inactive)]

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

Comment 2

[Christian Holler (:decoder)]

Attachment: Patch

Patch. Not tested on try yet, just trying to gather initial feedback whether this is the right thing :)

Comment 3

[Christian Holler (:decoder)]

Not adding any new annotations in this bug, so we don't need to hide this.

Comment 4

[Mats Palmgren (inactive)]

Comment on attachment 712953 [details] [diff] [review]
Patch

Looks great!

Comment 5

[Christian Holler (:decoder)]

Attachment: 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 6

[Mats Palmgren (inactive)]

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.

Comment 7

[Christian Holler (:decoder)]

(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?

Comment 8

[Mats Palmgren (inactive)]

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.)

Comment 9

[Nicholas Nethercote [inactive]]

> 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.

Comment 10

[Julian Seward [:jseward]]

(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.

Comment 11

[Christian Holler (:decoder)]

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.

Comment 12

[Jesse Ruderman]

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.

Comment 13

[Nicholas Nethercote [inactive]]

Keep it simple, please!  Just always poison on debug builds.

Comment 14

[Christian Holler (:decoder)]

Attachment: 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.

Comment 15

[Christian Holler (:decoder)]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Christian Holler (:decoder)]

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.

Comment 18

[Christian Holler (:decoder)]

Attachment: 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.

Comment 19

[Christian Holler (:decoder)]

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

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 21

[ISHIKAWA, Chiaki]

(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

Comment 22

[Christian Holler (:decoder)]

(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! :)

Comment 23

[ISHIKAWA, Chiaki]

(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