Page MenuHomePhabricator

scudo: Introduce a new mechanism to let Scudo access a platform-specific TLS slot
ClosedPublic

Authored by pcc on Sep 9 2020, 3:33 PM.

Details

Summary

An upcoming change to Scudo will change how we use the TLS slot
in tsd_shared.h, which will be a little easier to deal with if
we can remove the code path that calls pthread_getspecific and
pthread_setspecific. The only known user of this code path is Fuchsia.

We can't eliminate this code path by making Fuchsia use ELF TLS
because although Fuchsia supports ELF TLS, it is not supported within
libc itself. To address this, Roland McGrath on the Fuchsia team has
proposed that Scudo will optionally call a platform-provided function
to access a TLS slot reserved for Scudo. Android also has a reserved
TLS slot, but the code that accesses the TLS slot lives in Scudo.

We can eliminate some complexity and duplicated code by having Android
use the same mechanism that was proposed for Fuchsia, which is what
this change does. A separate change to Android implements it.

Diff Detail

Event Timeline

pcc created this revision.Sep 9 2020, 3:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2020, 3:33 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
pcc requested review of this revision.Sep 9 2020, 3:33 PM

Looks fine to me. Thanks!

compiler-rt/lib/scudo/standalone/tsd_shared.h
91

You could just make it static thread_local in this function and not need any other conditionalization declarations below.

196

Should these conditionals be #if !SCUDO_HAS_PLATFORM_TLS_SLOT now since the code above won't compile if that's not defined and this isn't declared?

Where is the implementation of getPlatformTlsSlot?

pcc added a comment.Sep 10 2020, 12:25 PM

Where is the implementation of getPlatformTlsSlot?

Here: https://android-review.googlesource.com/c/platform/bionic/+/1423770

pcc updated this revision to Diff 291064.Sep 10 2020, 12:31 PM
  • Address review comments
pcc marked an inline comment as done.Sep 10 2020, 12:32 PM
pcc added inline comments.
compiler-rt/lib/scudo/standalone/tsd_shared.h
91

Good idea, done.

196

Now moot

pcc updated this revision to Diff 291077.Sep 10 2020, 1:54 PM
pcc marked an inline comment as done.
  • Renamed the function per feedback on the Android review
cferris accepted this revision.Sep 10 2020, 4:52 PM

This seems fine to me, but maybe someone from llvm should also approve.

This revision is now accepted and ready to land.Sep 10 2020, 4:52 PM
cryptoad added inline comments.Sep 10 2020, 5:30 PM
compiler-rt/lib/scudo/standalone/tsd_shared.h
12

Is this still necessary?

16

Is there some requirement as to where this should live, or build time include path requirement that should be documented?

pcc updated this revision to Diff 291109.Sep 10 2020, 6:49 PM
  • Address review comments
compiler-rt/lib/scudo/standalone/tsd_shared.h
12

No it isn't, removed.

16

Added some documentation here.

cryptoad accepted this revision.Sep 10 2020, 7:06 PM
This revision was landed with ongoing or failed builds.Sep 10 2020, 7:16 PM
This revision was automatically updated to reflect the committed changes.
russell.gallop added inline comments.
compiler-rt/lib/scudo/standalone/tsd_shared.h
187

Is this still needed? The key is just created and deleted with nothing set or get. Apologies if I've missed something.

cryptoad added inline comments.Sep 21 2020, 12:52 PM
compiler-rt/lib/scudo/standalone/tsd_shared.h
187

That's a good point, it's no longer used, and we don't use the key destructor for the shared model.
I'll remove it in another CL.

russell.gallop added inline comments.Sep 22 2020, 1:40 AM
compiler-rt/lib/scudo/standalone/tsd_shared.h
187

Thanks