search

google / sanitizers

AddressSanitizer, ThreadSanitizer, MemorySanitizer
Other
11.41k stars 1.03k forks source link

Sanitizer requirements related to glibc thread descriptor/control block size #1382

Open fweimer-rh opened 3 years ago

fweimer-rh commented 3 years ago

We have a glibc patch to export another internal implementation for use by the sanitizers (specifically Leak Sanitizer apparently):

I'm not really fond of the patch because it seems to export internals so that the sanitizers compute something based on other internals. That is always going to be brittle. Unfortunately I do not know enough about sanitizer internals to figure out what they are doing with the data that they pull out of glibc.

We have an old proposal for a glibc API that presumably covers this space:

But it is callback-based, so its implementation is more or less guaranteed to have problems with locking, and also with multiple consumers within the same process.

I would like to restart this conversation and come up with an interface that works for the sanitizers and does not overly constrain libc implementations. Specifically, I'd like to know these details:

If you prefer, we can have a mailing list discussion about this (e.g., on libc-alpha). Thanks.

eugenis commented 3 years ago

We've implemented a similar API in bionic recently. You can find some discussions here:

https://android-review.googlesource.com/c/platform/bionic/+/1228403/14..36/libc/bionic/sys_thread_properties.cpp#b51 and this is the final change:

https://android-review.googlesource.com/c/platform/bionic/+/1360925/21/libc/include/sys/thread_properties.h

There are basically two uses for this info in sanitizers:

  1. clean tls shadow when a stack is reused for a new thread without being reallocated.
  2. find root pointers for leak detection.

(1) is done on thread start, from the same thread. (2) is done in a pseudo-thread after StopTheWorld. At this point all threads are frozen while holding malloc and dl locks, and we iterate over all threads in the process.

AFAIK we never use this API in a signal handler.

On Fri, Mar 5, 2021 at 4:53 AM Florian Weimer notifications@github.com wrote:

We have a glibc patch to export another internal implementation for use by the sanitizers (specifically Leak Sanitizer apparently):

I'm not really fond of the patch because it seems to export internals so that the sanitizers compute something based on other internals. That is always going to be brittle. Unfortunately I do not know enough about sanitizer internals to figure out what they are doing with the data that they pull out of glibc.

We have an old proposal for a glibc API that presumably covers this space:

But it is callback-based, so its implementation is more or less guaranteed to have problems with locking, and also with multiple consumers within the same process.

I would like to restart this conversation and come up with an interface that works for the sanitizers and does not overly constrain libc implementations. Specifically, I'd like to know these details:

  • Do the sanitizers need to access this information for the current thread only, or for all threads?
  • Is the information only needed at specific points of the process life time (e.g., during shutdown, similar to __libc_freeres), or continuously?
  • Is it necessary to access this information from signal handlers?

If you prefer, we can have a mailing list discussion about this (e.g., on libc-alpha). Thanks.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/google/sanitizers/issues/1382, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADG4SUAIR5LROXJYHTK2MDTCDH5ZANCNFSM4YVFL43Q .

oontvoo commented 3 years ago
  • Do the sanitizers need to access this information for the current thread only, or for all threads?

In case it wasn't clear, for static TLS, the API only needs to get the info for the current thread. For DSO TLS, it needs to find it for any given thread.

  • Is the information only needed at specific points of the process life time (e.g., during shutdown, similar to __libc_freeres), or continuously?

In the Bionic implementation, we assumed it could be accessed at any point, but it is undefined if there is concurrent modification to the region while we're iterating it.

Related libc-coord thread: https://www.openwall.com/lists/libc-coord/2020/03/05/1

fweimer-rh commented 3 years ago

@oontvoo With DSO TLS, do you mean the various dynamic ELF TLS variants? In some cases, backing store is allocated by malloc, so maybe you do not track this data right now because it merely appears as heap memory. But that is an implementation detail that might change. In addition to what is used to implement __thread/thread_local compiler support, there is also the pthread_key_create infrastructure. Presumably you need to scan the user pointers in there, but shadow stack coverage is not interesting because user code does not write directly there (only via pthread_setspecific).

@eugenis Your item (1) (clean TLS shadow) seems to assume that TLS data structures cannot move in memory and their size is fixed. That is not true in general. While we cannot move the static TLS area, we can grow it at the front or the end. Based on my understanding, the shadow data would have to be adjusted in this case. Would it be possible to do this lazily, after detecting a pointer violation, but before reporting it? And suppress the error if the access was to memory that has subsequently been made available as TLS data?

oontvoo commented 3 years ago

re: DSO TLS

Yeah, that's what I meant. (ie., TLS used/allocated by dynamically loaded modules. There's some descriptions about Bionic's implementation/layout here )

[...] In some cases, backing store is allocated by malloc, so maybe you do not track this data right now because it merely appears as heap memory. [...]

Yes-ish ... (For Bionic, DTLS memory is allocated via its allocator, so we do track and report it in the API)

there is also the pthread_key_create infrastructure. Presumably you need to scan the user pointers in there [...]

Yes, TSD is included in the TLS block returnt by the API

Your item (1) (clean TLS shadow) seems to assume that TLS data structures cannot move in memory and their size is fixed. That is not true in general. While we cannot move the static TLS area, we can grow it at the front or the end.

How can the static TLS area change? (IIRC, it's "static" because it's allocated once at the start and that's it) The only parts that grow(or shrink) is the D-TLS, yes?

fweimer-rh commented 3 years ago

How can the static TLS area change?

Static TLS usage may increase due to dlopen of a shared object that uses initial-exec TLS. In glibc, surplus static TLS is used for this. At present, all surplus static TLS is readable all the time, and its size is fixed, but it is conceivable that memory (as apposed to address space) for surplus static TLS is allocated on demand. Then the entire reserved address space area cannot be scanned for pointers initially, and the area that needs scanning can grow over time.

eugenis commented 3 years ago

On Tue, Mar 9, 2021 at 11:46 PM Florian Weimer @.***> wrote:

How can the static TLS area change?

Static TLS usage may increase due to dlopen of a shared object that uses initial-exec TLS. In glibc, surplus static TLS is used for this. At present, all surplus static TLS is readable all the time, and its size is fixed, but it is conceivable that memory (as apposed to address space) for surplus static TLS is allocated on demand. Then the entire reserved address space area cannot be scanned for pointers initially, and the area that needs scanning can grow over time.

In this case we would need to know about the new allocation. It could be reported through __libc_iterate_dynamic_tls - there is not much difference between this kind of "static" and dynamic TLS, sanitizer-wise.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/google/sanitizers/issues/1382#issuecomment-795035893, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADG4SQR6CZN3DBKVWPRM73TC4PWJANCNFSM4YVFL43Q .