This is an archive of the discontinued LLVM Phabricator instance.

[scudo] Avoid polluting the TLS slot used by libc-scudo
AbandonedPublic

Authored by oontvoo on Nov 19 2020, 2:35 PM.

Details

Summary

The change in aosp/1299034 resulted in scudoInit() not being called ever.

Tested:

********************
Unsupported Tests (4):
  Scudo-aarch64 :: preinit.c
  Scudo-aarch64 :: preload.cpp
  Scudo-aarch64 :: symbols.test
  Scudo-aarch64 :: valloc.c


Testing Time: 107.52s
  Unsupported:  4
  Passed     : 21
+ wait
+ for _arg in "$@"
+ local _arch=aarch64

Diff Detail

Event Timeline

oontvoo created this revision.Nov 19 2020, 2:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 19 2020, 2:35 PM
Herald added subscribers: Restricted Project, pengfei, jfb and 2 others. · View Herald Transcript
oontvoo requested review of this revision.Nov 19 2020, 2:35 PM
hctim added a comment.Nov 19 2020, 2:47 PM

This patch could still result in bugs. In this case, the platform scudo would have its TLS slot clobbered by test scudo. This can cause problems if libc scudo needs to do anything (like free() from a LTO-inlined call inside libc, or scudo_malloc_set_zero_contents), and its TLS slot was clobbered by test scudo.

oontvoo updated this revision to Diff 306819.Nov 20 2020, 7:24 PM

updated diff (avoid using the tls-slot reservered by bionic when SCUDO_HAS_PLATFORM_TLS_SLOT is not set)

oontvoo retitled this revision from [WIP] Move scudo-init out of dependency on the TLS that's also used by standalone-scudo. to [scudo] Avoid polluting the TLS slot used by libc-scudo.Nov 20 2020, 7:26 PM
oontvoo edited the summary of this revision. (Show Details)
pcc added a subscriber: pcc.Nov 20 2020, 7:30 PM

Can we not just drop support for Android in non-standalone Scudo? As far as I know it is only being used (if at all) on a limited set of platforms which does not include Android.

In D91824#2409331, @pcc wrote:

Can we not just drop support for Android in non-standalone Scudo? As far as I know it is only being used (if at all) on a limited set of platforms which does not include Android.

Now that Scudo replaced jemalloc in Bionic, the "old" Scudo shouldn't be used anywhere on Android anymore, it should be safe to remove support. @cferris what do you think?

In D91824#2409331, @pcc wrote:

Can we not just drop support for Android in non-standalone Scudo? As far as I know it is only being used (if at all) on a limited set of platforms which does not include Android.

Now that Scudo replaced jemalloc in Bionic, the "old" Scudo shouldn't be used anywhere on Android anymore, it should be safe to remove support. @cferris what do you think?

That seems fine to me, but isn't this used for asan and hwasan which are still used on Android?

Yes, the TLS slot is owned by the platform heap allocator (scudo/asan/hwasan). Tests should not touch it.

pcc added a comment.Dec 1 2020, 2:54 PM

I think @cferris was asking whether old scudo was still being used by asan/hwasan. To which the answer is "no" I believe, the sanitizers have their own independent copy of the allocator.

In D91824#2426891, @pcc wrote:

I think @cferris was asking whether old scudo was still being used by asan/hwasan. To which the answer is "no" I believe, the sanitizers have their own independent copy of the allocator.

Just to wrap up the threads here, the consensus is that all the non-standalone stuff under compiler-rt/scudo/* should simply be deleted? (along with their tests?)

In D91824#2426891, @pcc wrote:

I think @cferris was asking whether old scudo was still being used by asan/hwasan. To which the answer is "no" I believe, the sanitizers have their own independent copy of the allocator.

Just to wrap up the threads here, the consensus is that all the non-standalone stuff under compiler-rt/scudo/* should simply be deleted? (along with their tests?)

Hnmm I was going for Android support for Scudo non-standalone should be dropped, but some people are still working on this.

oontvoo abandoned this revision.Jan 5 2021, 11:18 AM

retiring this in favour of hctim's patch