This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer] Use -Wl,-z,global on Android
ClosedPublic

Authored by cryptoad on Jul 11 2018, 11:44 AM.

Details

Summary

Use -Wl,-z,global for all Sanitizer shared libraries on
Android. We want them to be in the global group
(https://android.googlesource.com/platform/bionic/+/master/android-changes-for-ndk-developers.md#changes-to-library-search-order)
to avoid any alloc/dealloc mismatch between the libc allocator & said library.

audioserver was one of the binary that exhibited the problem with Scudo,
this seems to fix it.

[edited for accuracy]

Event Timeline

cryptoad created this revision.Jul 11 2018, 11:44 AM
Herald added subscribers: Restricted Project, delcypher, mgorny, srhines. · View Herald TranscriptJul 11 2018, 11:44 AM
cryptoad edited the summary of this revision. (Show Details)Jul 11 2018, 12:13 PM
cryptoad edited the summary of this revision. (Show Details)
cryptoad updated this revision to Diff 155044.Jul 11 2018, 12:17 PM

Updating comment.

eugenis added inline comments.Jul 12 2018, 11:05 AM
lib/scudo/CMakeLists.txt
24

ASAN_DYNAMIC_LINK_FLAGS <- is this a typo?

Can this be done in a common code for all sanitizers that intercept malloc?

Also, I don't think github is the canonical source for AOSP, could you update the link in the patch description to https://android.googlesource.com/platform/bionic/+/master/android-changes-for-ndk-developers.md ?

cryptoad edited the summary of this revision. (Show Details)Jul 12 2018, 11:08 AM
cryptoad updated this revision to Diff 155227.Jul 12 2018, 11:10 AM

s/ASAN/SCUDO because I failed at copy/pasting :(

cryptoad added inline comments.Jul 12 2018, 11:13 AM
lib/scudo/CMakeLists.txt
24

Doh! Thanks for catching the error.
Name updated, description updated.
Regarding the last point, you'd want an update to SANITIZER_COMMON_LINK_FLAGS based on interception? (not sure if it's possible, just trying to clarify).

cryptoad edited the summary of this revision. (Show Details)Jul 12 2018, 11:14 AM
eugenis added inline comments.Jul 12 2018, 11:35 AM
lib/scudo/CMakeLists.txt
24

SANITIZER_COMMON_LINK_FLAGS can't depend on interception, I think.

In fact, adding -z,global unconditionally should not break anything as far as I can see.
Could you move this code to the top-level (compiler-rt) CMakeLists, and remove it from asan and hwasan?

cryptoad updated this revision to Diff 155245.Jul 12 2018, 11:52 AM

Moving the "global" link flag to the common CMake code so that it can benefit
all the shared libraries. Remove the previous instances of said code in ASan
and HWAsan.

cryptoad marked 3 inline comments as done.Jul 12 2018, 11:53 AM
cryptoad added inline comments.
lib/scudo/CMakeLists.txt
24

Done, will test locally to make sure it all works out.

cryptoad retitled this revision from [scudo] Use -Wl,-z,global on Android to [sanitizer] Use -Wl,-z,global on Android.Jul 12 2018, 11:58 AM
cryptoad edited the summary of this revision. (Show Details)
eugenis accepted this revision.Jul 12 2018, 12:11 PM

LGTM, thanks!

This revision is now accepted and ready to land.Jul 12 2018, 12:11 PM
This revision was automatically updated to reflect the committed changes.
cryptoad marked an inline comment as done.