Page MenuHomePhabricator

[GWP-ASan] Update alignment on Android.
ClosedPublic

Authored by hctim on Feb 10 2020, 3:10 PM.

Details

Summary

Android has different alignment requirements. You can read more about
them here
(https://cs.android.com/android/platform/superproject/+/master:bionic/tests/malloc_test.cpp;l=808),
but the general gist is that for malloc(x <= 8), we do malloc(8), and
for everything else, we do 16-byte alignment.

Diff Detail

Event Timeline

hctim created this revision.Feb 10 2020, 3:10 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 10 2020, 3:10 PM
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript
eugenis accepted this revision.Feb 10 2020, 3:29 PM

LGTM

compiler-rt/lib/gwp_asan/platform_specific/utilities_posix.cpp
17

missing __ at the end

57

static constexpr

62

rename to "rightAlignedAllocationSize"? Otherwise it's not clear what the allocation size has to do with alignment.

compiler-rt/lib/gwp_asan/utilities.h
19

alignment is usually the actual number in bytes.
Call this AlignmentPolicy or something.

This revision is now accepted and ready to land.Feb 10 2020, 3:29 PM

Does Bionic's malloc have the required alignment information available at the call to gwp_asan::allocate? If so, we could get tighter right alignment by adding an alignment parameter (instead of conservative 16-byte rounding). This is the approach used in both Chrome and TCMalloc.

e.g., one of the most common malloc sizes is 24 (nodes in tree structures), which won't get perfectly right-aligned with the current approach, though it should be safe to do so.

hctim added a comment.Feb 10 2020, 6:09 PM

Does Bionic's malloc have the required alignment information available at the call to gwp_asan::allocate? If so, we could get tighter right alignment by adding an alignment parameter (instead of conservative 16-byte rounding). This is the approach used in both Chrome and TCMalloc.

e.g., one of the most common malloc sizes is 24 (nodes in tree structures), which won't get perfectly right-aligned with the current approach, though it should be safe to do so.

Unfortunately not, we're called directly from the malloc() family. Doesn't matter though, because...

As @pcc pointed out, the Android policy isn't eight-then-sixteen, it's actually "always align to 16 bytes if asked for an allocation with alignment 16-bytes, otherwise align 8-bytes", e.g.

malloc(5) => malloc(8)
malloc(9) => malloc(16)
malloc(17) => malloc(24)
malloc(16) => malloc(16), or if you're the backing allocator and there are no 16-byte buckets, you *must* use malloc(32), not malloc(24).

For our purposes, we can *always* ensure that a 16-byte allocation gets 16-bytes, and we only need to round to 8-bytes. Let me update the patchset.

hctim updated this revision to Diff 244035.Feb 11 2020, 5:04 PM
hctim marked 4 inline comments as done.
  • Update with eugenis@ comments and 8-byte alignment.
hctim added a comment.Feb 12 2020, 8:29 AM

Just wanted to check, still LGTY @morehouse @eugenis?

morehouse added inline comments.Feb 12 2020, 11:01 AM
compiler-rt/lib/gwp_asan/options.inc
20–23

I'm not positive if its safe to go down to 8 bytes here. Until recently, 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 news 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...

22

Add space after .

compiler-rt/lib/gwp_asan/platform_specific/utilities_posix.cpp
14

Is this Bionic-specific, or Android generally?

40

static

46

static

54

Should this be the same for Android generally?

56

I think we should have a comment here explaining why rounding to 8 is sufficient for Android.

compiler-rt/lib/gwp_asan/tests/alignment.cpp
28

The test is called AlignToEight, but we expect alignments of 16, 24, etc.... Maybe call it AndroidAlignment and just use 8 or 16, since that's what we need.

And maybe add a comment explaining Android's alignment scheme.

morehouse added inline comments.Feb 12 2020, 11:09 AM
compiler-rt/lib/gwp_asan/options.inc
20–23

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.

hctim marked 12 inline comments as done.Feb 12 2020, 2:11 PM
hctim added inline comments.
compiler-rt/lib/gwp_asan/options.inc
20–23

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.

compiler-rt/lib/gwp_asan/platform_specific/utilities_posix.cpp
14

bionic-specific

54

No, we're specifically targeting Android Bionic here (and their guarantees). Android libc (for host) is a whole different kettle of fish.

56

Done above alignBionic.

hctim updated this revision to Diff 244278.Feb 12 2020, 2:11 PM
hctim marked 3 inline comments as done.
  • Fixed Matt's comments, and added some documentation.
morehouse accepted this revision.Feb 12 2020, 3:16 PM
morehouse added inline comments.
compiler-rt/lib/gwp_asan/tests/alignment.cpp
28

Nit: we technically don't care about alignments >16, so why test for them? (same for PowerOfTwo test)

hctim marked 2 inline comments as done.Feb 12 2020, 3:23 PM
hctim added inline comments.
compiler-rt/lib/gwp_asan/tests/alignment.cpp
28

We're not testing the alignment here, it's actually the mapping between asked size => real (aligned) size. Let me update the variable names to make this more clear.

hctim updated this revision to Diff 244286.Feb 12 2020, 3:24 PM
hctim marked an inline comment as done.
  • /s/AllocSizeToAlignment/AskedSizeToAlignedSize
This revision was automatically updated to reflect the committed changes.
cryptoad added inline comments.
compiler-rt/lib/gwp_asan/platform_specific/utilities_posix.cpp
87

I am getting a gcc error here:
control reaches end of non-void function [-Werror=return-type]

hctim marked 2 inline comments as done.Feb 13 2020, 10:24 AM
hctim added inline comments.
compiler-rt/lib/gwp_asan/platform_specific/utilities_posix.cpp
87

gcc's warning is wrong here (all paths lead to return), but i can fix it anyway.