Re: [PATCH] aarch64: Remove ld.so __tls_get_addr plt usage

[enh <enh@google.com>]

bionic implemented
https://sourceware.org/glibc/wiki/ThreadPropertiesAPI in
https://android.googlesource.com/platform/bionic/+/refs/heads/main/libc/include/sys/thread_properties.h
but tbh i'm not sure that the sanitizer folks moved over to the new
api?

(i don't think we could just use malloc() because jemalloc -- which we
still haven't fully removed for very low-end users in favor of scudo
-- was itself using TLS thread locals? scudo has its own reserved
constant slot in bionic, so that should be fine, i think, but we're
not there yet.)

On Wed, Apr 10, 2024 at 1:24 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Fangrui Song:
>
> > Last time I analyzed the __tls_get_addr interceptor in sanitizers, I
> > have made quite some notes at
> > https://maskray.me/blog/2021-02-14-all-about-thread-local-storage#why-does-compiler-rt-need-to-know-tls-blocks
> >
> > Yes, an interceptor is needed.
>
> There's no guarantuee that TLS access goes through a regular function
> call, so any design that relies on such a call happening is
> fundamentally broken.
>
> Quoting from your article:
>
> | Note: if the allocation is rtld/libc internal and not intercepted,
> | there is no need to unpoison the range. The associated shadow is
> | supposed to be zeros. However, if the allocation is intercepted, the
> | runtime should unpoison the range in case the range reuses a previous
> | allocation which happens to contain poisoned bytes.
> |
> | In glibc, _dl_allocate_tls and _dl_deallocate_tls call malloc/free
> | functions which are internal and not intercepted, so the allocations
> | are opaque to the runtime and the shadow bytes are all zeroes.
>
> I don't think this is accurate.  We call the application malloc/free for
> non-main threads after initialization.
>
> Having an accurate description of sanitizer needs in this area would be
> really helpful, but I think we are not quite there yet.  (This is
> different from an API description.)
>
> I think there are several aspects here:
>
> (a) Avoid false errors for bounds checks for Address Sanitizer.
>
> (b) Support pointer discovery for Leak Sanitizer (essentially conservative
>     garbage collection).
>
> (c) Avoid false data race reports for Thread Sanitizer after TLS reuse
>     from one thread for a different thread (only with non-overlapping
>     lifetimes).
>
> Based on your description, I'm not sure if (a) is actually a problem.
> If we don't use application malloc for TLS allocations, bounds checking
> is bypassed apparently?  And if we use malloc, out-of-bounds accesses
> would be actual bugs.
>
> Aspect (b) is a real issue.  Could we address that by allocating the TCB
> (with static TLS) and all dynamic TLS with application malloc (or
> rather, memalign/aligned_alloc), and keep a pointer to the allocation on
> the thread stack?  Then a conservative collector could find it, and scan
> it for pointers.  A gap remains for the main thread, whose TCB is not
> allocated using application malloc—and can't be, as the application
> malloc itself very likely depends on the TCB already being there.  We
> could switch TCBs after allocating another one with malloc, but that
> would require some hand-off protocol, I believe.  Maybe it's better to
> register early allocations with the sanitizer directly, using some
> appropriate API.
>
> For (c), we could just stop caching TCBs after thread exit.  If we call
> free, and reallocate for the new thread, that should avoid the false
> data race.  This issue does not affect the main thread.
>
> Based on that, I don't think we need to support discovery of TLS areas,
> or export any other internal implementation details.  We just need to
> use more malloc within glibc if we detect an active sanitizer, and find
> a way to make the TCB allocation of the main thread known to the
> sanitizer.
>
> Thanks,
> Florian
>