[PATCH] D19921: [esan] EfficiencySanitizer shadow memory

[Derek Bruening via llvm-commits]

bruening marked 5 inline comments as done.
bruening added a comment.

> That is more complex, though. Both me and Mike asked about this, so it seems like a better explanation would be nice.


I think you are overlooking the very similar complexities in the other sanitizers.  ASan has to use a different offset to handle PIE versus the offset it uses for non-PIE, which is a complexity I do not want.  Each of the other sanitizers has its own, different, set of offsets.  Our mapping handles PIE and vsyscall and handles multiple scales to support multiple tools, all with one formula with two instances of tweaked offset.

> I am not seeing a reason for the different offsets. I understand it's all a simple transformation to get each offset, but the "why" still eludes me (can't talk for Mike, though).


This is explained in esan_shadow.h: for two scale cases, the base offset produces a shadow(shadow) conflict.  Tweaking it satisfies the shadow(shadow) property, helping to avoid wild accesses by the app clobbering a tool's own metadata.  It makes the tools more robust.


================
Comment at: lib/esan/esan.cpp:72
@@ +71,3 @@
+      DCHECK(isAppMem(AppStart));
+      DCHECK(!isAppMem(AppStart - 1));
+      DCHECK(isAppMem(AppEnd - 1));
----------------
filcab wrote:
> Not really. ASan has one special case where there's more than a shadow region.
> All the others have one shadow region (divided into low shadow, high shadow, and shadow gap).
> 
> You mention that "Any direct-map shadow approach has to use different shadow regions for the different 64-bit app regions", what's missing is the "why?".
> 
> I might be missing something, but like I said, I don't see a reason to have all this complexity.
> Having a single, big, shadow region would also help with not needing to mask addresses in the memToShadow transformation. This also avoids having problems in the `appToShadow(appToShadow(x))` transformation, as that hits the shadow gap, which has no read nor write permissions.
Low shadow and high shadow are **//two different regions//**.  There is not a third region for PIE in that same scheme because ASan has to use **//a different offset//** when PIE is present, resulting in a completely different mapping.  There is as much complexity there as in our mapping here, yet ours handles multiples scales and it handles PIE and the vsyscall page.

Look at the application memory ranges: they are distinct and far apart.  Using a linear mapping that preserves their identities places them into distinct shadow ranges.

================
Comment at: lib/esan/esan.cpp:74
@@ +73,3 @@
+      DCHECK(isAppMem(AppEnd - 1));
+      DCHECK(!isAppMem(AppEnd));
+      DCHECK(!isShadowMem(AppStart));
----------------
filcab wrote:
> Right, you have white-listed some app memory, and we end up with `appToShadow(AppEnd)` not falling into shadow memory because there are blanks in the address space that aren't "app" nor "shadow", is that so?
> 
Anything outside of the listed regions is disallowed.

================
Comment at: lib/esan/esan_shadow.h:48
@@ +47,3 @@
+// We use Umbra's scheme as it was designed to support different
+// offsets, it supports two different shadow mappings (which we may want to
+// use for future tools), and it ensures that the shadow of a shadow will
----------------
filcab wrote:
> Nothing in this patch mentions "why" supporting different offsets is a priority. Is it not possible to do just the one offset (with possibly varying scales) on Linux due to virtual address space fragmentation?
> Are there performance concerns which we can address when we scale down the shadow, but can't when the shadow is very large?
> 
> I really want to avoid this code being more complex than it needs to. Especially since it will likely change among different OS.
Please read the full comment.  This is explained.  Pasting:

```
56              // For other scales, the
57		​// offset is shifted left by the scale, except for scales of 1 and 2 where
58		​// it must be tweaked in order to pass the double-shadow test
59		​// (see the "shadow(shadow)" comments below):"
```

No, it is not possible to use one offset.

Again, there is as much complexity in the other sanitizer shadow mappings.  They have special offsets as well, and they all have to be tweaked on different platforms.

If you can come up with a linear mapping involving a scale and an add that uses a single offset for every scale and avoids colliding with anything (including shadow(shadow)), please let me know.


http://reviews.llvm.org/D19921