[PATCH] D74364: [GWP-ASan] Update alignment on Android.

[Mitch Phillips via Phabricator via llvm-commits]

hctim marked 12 inline comments as done.
hctim added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/options.inc:19
     "the page boundary? By default (false), we round up allocation size to the "
-    "nearest power of two (1, 2, 4, 8, 16) up to a maximum of 16-byte "
-    "alignment for performance reasons. Setting this to true can find single "
-    "byte buffer-overflows for multibyte allocations at the cost of "
-    "performance, and may be incompatible with some architectures.")
+    "nearest power of two (1, 2, 4, 8) up to a maximum of 8-byte alignment for "
+    "performance reasons. On Android, we use 8-byte alignment by "
----------------
morehouse wrote:
> morehouse wrote:
> > I'm not positive if its safe to go down to 8 bytes here.  Until [recently](http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2293.htm), there was an ambiguity in the standard that meant most mallocs used 16-byte alignment.  It's gone now, but there may still be Android use cases that expect `malloc(16+)` to have the higher alignment (pretty sure this exists in Google3).
> > 
> > In TCMalloc, all `operator new`s get 8-byte alignment by default while `malloc` and friends follow the `memalign(16, size)` path.  But  I don't know how Bionic does this...
> Regardless, I think we'll need 16-byte alignment for malloc() with size>=16.  Unless `malloc()` doesn't follow the path with GWP-ASan sampling.
The "source of truth" for bionic is documented here: https://cs.android.com/android/platform/superproject/+/master:bionic/tests/malloc_test.cpp;l=808

So, in general, we currently are weak-aligned (with the current power-of-two alignment) for all platforms.

Android requires semi-strong alignment, where:
```
malloc(0 < x <= 8) => malloc(8)/malloc(16)/malloc(24)...

malloc(8 < x <= 16) => malloc(16)/malloc(32)/malloc(48)...

malloc(16 < x <= 24) => malloc(16)/malloc(24)/malloc(32)...

malloc(24 < x <= 32) => malloc(32)/malloc(48)/malloc(64)...
``` 

This guarantee is enforced by the aforementioned `malloc.align_check` test.

For GWP-ASan on Android, this mish-mash can be greatly simplified. As `malloc(X)` can *always* get exactly an `X`-sized allocation, we don't need to worry about the `malloc(24 < x <= 32)` case. This simplifies down to us only requiring 8-byte alignment.

I definitely agree we should consider enforcing strong alignment for general platforms. I'd like to (in future) change this option to use either `perfect`, `strong`, `weak` (current), or `android` alignment. I'll follow up with that later, but for now, I've just changed some wording to basically add an Android-specific strategy. The default (weak alignment) on all other platforms hasn't changed.


================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/utilities_posix.cpp:14
+
+#ifdef __BIONIC__
 #include <stdlib.h>
----------------
morehouse wrote:
> Is this Bionic-specific, or Android generally?
bionic-specific


================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/utilities_posix.cpp:54
+
+#ifdef __BIONIC__
+static constexpr AlignmentStrategy PlatformDefaultAlignment =
----------------
morehouse wrote:
> Should this be the same for Android generally?
No, we're specifically targeting Android Bionic here (and their guarantees). Android libc (for host) is a whole different kettle of fish.


================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/utilities_posix.cpp:56
+static constexpr AlignmentStrategy PlatformDefaultAlignment =
+    AlignmentStrategy::EIGHT;
+#else  // __BIONIC__
----------------
morehouse wrote:
> I think we should have a comment here explaining why rounding to 8 is sufficient for Android.
Done above alignBionic.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74364/new/

https://reviews.llvm.org/D74364