[PATCH] D74364: [GWP-ASan] Update alignment on Android.
Mitch Phillips via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 12 14:15:09 PST 2020
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
More information about the llvm-commits
mailing list