This is an archive of the discontinued LLVM Phabricator instance.

[GWP-ASan] Fuchsia specific mapping & utilities functions
ClosedPublic

Authored by cryptoad on Oct 30 2020, 10:44 AM.

Details

Summary

This CL introduces the Fuchsia versions of the existing platform
specific functions.

For Fuchsia, we need to track the VMAR (https://fuchsia.dev/fuchsia-src/reference/kernel_objects/vm_address_region)
of the Guarded Pool mapping, and for this purpose I added some platform
specific data structure that remains empty on POSIX platforms.

getThreadID is not super useful for Fuchsia so it's just left as a
stub for now.

While testing the changes in my Fuchsia tree, I realized that
guarded_pool_allocator_tls.h should have closed the namespace before
including GWP_ASAN_PLATFORM_TLS_HEADER, otherwise drama ensues.

This was tested in g3, upstream LLVM, and Fuchsia (with local changes).

Diff Detail

Event Timeline

cryptoad created this revision.Oct 30 2020, 10:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 30 2020, 10:44 AM
Herald added subscribers: Restricted Project, phosek, mgorny. · View Herald Transcript
cryptoad requested review of this revision.Oct 30 2020, 10:44 AM
cryptoad updated this revision to Diff 301961.Oct 30 2020, 10:51 AM

Couple of small fixes.

hctim accepted this revision.Oct 30 2020, 1:37 PM

LGTM - I'll let Roland confirm the Fuchsia-specific memory parts.

This revision is now accepted and ready to land.Oct 30 2020, 1:37 PM
mcgrathr accepted this revision.Oct 30 2020, 3:54 PM

LGTM with a few minor changes.
Thanks!

compiler-rt/lib/gwp_asan/platform_specific/common_fuchsia.cpp
13

Perhaps comment that on Fuchsia this is only used for AllocationTrace.ThreadID and allocation traces are not yet supported on Fuchsia.

compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_fuchsia.cpp
24

This is fine but I don't see why it's using the separate local and copying instead of passing &getThreadLocals()->RandomState.

30

This VMO does not need to be resizable.
It can just use 0 flags here.

37

Is it possible for the failure message to include Status or zx_status_get_string(Status)?

75

Doesn't need to be resizable.

103

Use ZX_PAGE_SIZE from <zircon/limits.h> instead.

compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_fuchsia.h
10

It seems weird to have any #if outside the header guard. Obviously inverting the order makes no practical difference but I think it makes the code more readable to have it inside and separated by a blank line.

cryptoad updated this revision to Diff 302062.Oct 30 2020, 9:21 PM
cryptoad marked 5 inline comments as done.

Addressing Roland's comments.

cryptoad added inline comments.Oct 30 2020, 9:25 PM
compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_fuchsia.cpp
37

I'd probably take that as an improvement for later as current check & die only take a string and there is no string formatting function in GWP-ASan.

compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_fuchsia.h
10

Right, that was initially the way I went, but in the previous review, Mitch suggested the other way.
I can go one way or the other if we have a consensus.

mcgrathr added inline comments.Oct 30 2020, 10:14 PM
compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_fuchsia.cpp
37

Understood.

compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_fuchsia.h
10

On all style/cosmetic points I defer to the primary maintainer.

This revision was landed with ongoing or failed builds.Oct 31 2020, 10:23 AM
This revision was automatically updated to reflect the committed changes.