This is an archive of the discontinued LLVM Phabricator instance.

Differential D49198

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

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

Details

Reviewers
eugenis
alekseyshl
Commits
rG4a547fb62d40: [sanitizer] Use -Wl,-z,global on Android
rL337010: [sanitizer] Use -Wl,-z,global on Android
rCRT337010: [sanitizer] Use -Wl,-z,global on Android
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]

Diff Detail

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
Harbormaster completed remote builds in B20272: Diff 155040.Jul 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.

Harbormaster completed remote builds in B20273: Diff 155044.Jul 11 2018, 12:17 PM
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 :(

Harbormaster completed remote builds in B20311: Diff 155227.Jul 12 2018, 11:10 AM
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.

Herald added a subscriber: kubamracek. · View Herald TranscriptJul 12 2018, 11:52 AM
Harbormaster completed remote builds in B20317: Diff 155245.Jul 12 2018, 11:52 AM
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
Closed by commit rCRT337010: [sanitizer] Use -Wl,-z,global on Android (authored by cryptoad). · Explain WhyJul 13 2018, 8:36 AM
This revision was automatically updated to reflect the committed changes.
cryptoad marked an inline comment as done.
grimar mentioned this in D49374: [LLF][ELF] - Support -z global.Jul 16 2018, 7:19 AM
grimar mentioned this in rL340802: [LLF][ELF] - Support -z global..Aug 28 2018, 1:26 AM
grimar mentioned this in rLLD340802: [LLF][ELF] - Support -z global..

Revision Contents

PathSize
lib/
scudo/
12 lines

Diff 155040

lib/scudo/CMakeLists.txt

Show All 11 Lines
append_list_if(COMPILER_RT_HAS_LIBRT rt SCUDO_MINIMAL_DYNAMIC_LIBS) append_list_if(COMPILER_RT_HAS_LIBRT rt SCUDO_MINIMAL_DYNAMIC_LIBS)
append_list_if(COMPILER_RT_HAS_LIBPTHREAD pthread SCUDO_MINIMAL_DYNAMIC_LIBS) append_list_if(COMPILER_RT_HAS_LIBPTHREAD pthread SCUDO_MINIMAL_DYNAMIC_LIBS)
append_list_if(COMPILER_RT_HAS_LIBLOG log SCUDO_MINIMAL_DYNAMIC_LIBS) append_list_if(COMPILER_RT_HAS_LIBLOG log SCUDO_MINIMAL_DYNAMIC_LIBS)
set(SCUDO_DYNAMIC_LINK_FLAGS ${SANITIZER_COMMON_LINK_FLAGS}) set(SCUDO_DYNAMIC_LINK_FLAGS ${SANITIZER_COMMON_LINK_FLAGS})
# Use gc-sections by default to avoid unused code being pulled in. # Use gc-sections by default to avoid unused code being pulled in.
list(APPEND SCUDO_DYNAMIC_LINK_FLAGS -Wl,--gc-sections) list(APPEND SCUDO_DYNAMIC_LINK_FLAGS -Wl,--gc-sections)
if(ANDROID)
# On Android, -z global does not do what it is documented to do.
# On Android, -z global moves the library ahead in the lookup order, placing it
# right after the LD_PRELOADs. This is used to compensate for the fact that
# Android linker does not look at the dependencies of the main executable that
eugenisUnsubmitted
Done
ReplyInline Actions

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 ?

eugenis: ASAN_DYNAMIC_LINK_FLAGS <- is this a typo? Can this be done in a common code for all…
cryptoadAuthorUnsubmitted
Done
ReplyInline Actions

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: Doh! Thanks for catching the error. Name updated, description updated. Regarding the last point…
eugenisUnsubmitted
Done
ReplyInline Actions

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?

eugenis: SANITIZER_COMMON_LINK_FLAGS can't depend on interception, I think. In fact, adding -z,global…
cryptoadAuthorUnsubmitted
Not Done
ReplyInline Actions

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

cryptoad: Done, will test locally to make sure it all works out.
# aren't dependencies of the current DSO when resolving symbols from said DSO.
# The above is applicable to L MR1 or newer.
if (COMPILER_RT_HAS_Z_GLOBAL)
list(APPEND ASAN_DYNAMIC_LINK_FLAGS -Wl,-z,global)
endif()
endif()
# The minimal Scudo runtime does not inlude the UBSan runtime. # The minimal Scudo runtime does not inlude the UBSan runtime.
set(SCUDO_MINIMAL_OBJECT_LIBS set(SCUDO_MINIMAL_OBJECT_LIBS
RTSanitizerCommonNoTermination RTSanitizerCommonNoTermination
RTSanitizerCommonLibc RTSanitizerCommonLibc
RTInterception) RTInterception)
set(SCUDO_OBJECT_LIBS ${SCUDO_MINIMAL_OBJECT_LIBS}) set(SCUDO_OBJECT_LIBS ${SCUDO_MINIMAL_OBJECT_LIBS})
set(SCUDO_DYNAMIC_LIBS ${SCUDO_MINIMAL_DYNAMIC_LIBS}) set(SCUDO_DYNAMIC_LIBS ${SCUDO_MINIMAL_DYNAMIC_LIBS})
▲ Show 20 Lines • Show All 107 Lines • Show Last 20 Lines