From b1bd52cd0d8627df1187448b8247a9c7a4675019 Mon Sep 17 00:00:00 2001 From: Thurston Dang Date: Wed, 12 Apr 2023 20:53:49 +0000 Subject: [PATCH] Fix tls_get_addr handling for glibc >=2.25 This changes the sanitizers' tls_get_addr handling from a heuristic check of __signal_safe_memalign allocations (which has only been used in a since deprecated version of Google's runtime), to using the sanitizers' interface function to check if it is a malloc allocation (used since glibc >= 2.25). This is one of the approaches proposed by Keno in https://github.com/google/sanitizers/issues/1409#issuecomment-1214244142 This moves the weak annotation of __sanitizer_get_allocated_size/begin from the header to sanitizer_tls_get_addr.cpp, as suggested by Vitaly in D148060. Reviewed By: vitalybuka Differential Revision: https://reviews.llvm.org/D147459 --- .../sanitizer_allocator_interface.h | 4 +-- .../sanitizer_tls_get_addr.cpp | 29 ++++++++++--------- .../sanitizer_common/sanitizer_tls_get_addr.h | 26 +++++++++++------ compiler-rt/test/msan/dtls_test.c | 4 --- 4 files changed, 34 insertions(+), 29 deletions(-) diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_allocator_interface.h b/compiler-rt/lib/sanitizer_common/sanitizer_allocator_interface.h index 504109e9d3f6f..8f3b71eb6ce74 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_allocator_interface.h +++ b/compiler-rt/lib/sanitizer_common/sanitizer_allocator_interface.h @@ -21,8 +21,8 @@ extern "C" { SANITIZER_INTERFACE_ATTRIBUTE uptr __sanitizer_get_estimated_allocated_size(uptr size); SANITIZER_INTERFACE_ATTRIBUTE int __sanitizer_get_ownership(const void *p); -SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE const void * -__sanitizer_get_allocated_begin(const void *p); +SANITIZER_INTERFACE_ATTRIBUTE const void *__sanitizer_get_allocated_begin( + const void *p); SANITIZER_INTERFACE_ATTRIBUTE uptr __sanitizer_get_allocated_size(const void *p); SANITIZER_INTERFACE_ATTRIBUTE uptr __sanitizer_get_current_allocated_bytes(); diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_tls_get_addr.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_tls_get_addr.cpp index b13e2dc9e3327..252979f1c2baa 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_tls_get_addr.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_tls_get_addr.cpp @@ -12,6 +12,7 @@ #include "sanitizer_tls_get_addr.h" +#include "sanitizer_allocator_interface.h" #include "sanitizer_atomic.h" #include "sanitizer_flags.h" #include "sanitizer_platform_interceptors.h" @@ -26,13 +27,6 @@ struct TlsGetAddrParam { uptr offset; }; -// Glibc starting from 2.19 allocates tls using __signal_safe_memalign, -// which has such header. -struct Glibc_2_19_tls_header { - uptr size; - uptr start; -}; - // This must be static TLS __attribute__((tls_model("initial-exec"))) static __thread DTLS dtls; @@ -108,6 +102,14 @@ static const uptr kDtvOffset = 0x800; static const uptr kDtvOffset = 0; #endif +extern "C" { +SANITIZER_WEAK_ATTRIBUTE +uptr __sanitizer_get_allocated_size(const void *p); + +SANITIZER_WEAK_ATTRIBUTE +const void *__sanitizer_get_allocated_begin(const void *p); +} + DTLS::DTV *DTLS_on_tls_get_addr(void *arg_void, void *res, uptr static_tls_begin, uptr static_tls_end) { if (!common_flags()->intercept_tls_get_addr) return 0; @@ -125,19 +127,18 @@ DTLS::DTV *DTLS_on_tls_get_addr(void *arg_void, void *res, atomic_load(&number_of_live_dtls, memory_order_relaxed)); if (dtls.last_memalign_ptr == tls_beg) { tls_size = dtls.last_memalign_size; - VReport(2, "__tls_get_addr: glibc <=2.18 suspected; tls={0x%zx,0x%zx}\n", + VReport(2, "__tls_get_addr: glibc <=2.24 suspected; tls={0x%zx,0x%zx}\n", tls_beg, tls_size); } else if (tls_beg >= static_tls_begin && tls_beg < static_tls_end) { // This is the static TLS block which was initialized / unpoisoned at thread // creation. VReport(2, "__tls_get_addr: static tls: 0x%zx\n", tls_beg); tls_size = 0; - } else if ((tls_beg % 4096) == sizeof(Glibc_2_19_tls_header)) { - // We may want to check gnu_get_libc_version(). - Glibc_2_19_tls_header *header = (Glibc_2_19_tls_header *)tls_beg - 1; - tls_size = header->size; - tls_beg = header->start; - VReport(2, "__tls_get_addr: glibc >=2.19 suspected; tls={0x%zx 0x%zx}\n", + } else if (const void *start = + __sanitizer_get_allocated_begin((void *)tls_beg)) { + tls_beg = (uptr)start; + tls_size = __sanitizer_get_allocated_size(start); + VReport(2, "__tls_get_addr: glibc >=2.25 suspected; tls={0x%zx,0x%zx}\n", tls_beg, tls_size); } else { VReport(2, "__tls_get_addr: Can't guess glibc version\n"); diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_tls_get_addr.h b/compiler-rt/lib/sanitizer_common/sanitizer_tls_get_addr.h index a599c0bbc75cc..0ddab61deb102 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_tls_get_addr.h +++ b/compiler-rt/lib/sanitizer_common/sanitizer_tls_get_addr.h @@ -12,16 +12,24 @@ // the lack of interface that would tell us about the Dynamic TLS (DTLS). // https://sourceware.org/bugzilla/show_bug.cgi?id=16291 // -// The matters get worse because the glibc implementation changed between -// 2.18 and 2.19: -// https://groups.google.com/forum/#!topic/address-sanitizer/BfwYD8HMxTM -// -// Before 2.19, every DTLS chunk is allocated with __libc_memalign, +// Before 2.25: every DTLS chunk is allocated with __libc_memalign, // which we intercept and thus know where is the DTLS. -// Since 2.19, DTLS chunks are allocated with __signal_safe_memalign, -// which is an internal function that wraps a mmap call, neither of which -// we can intercept. Luckily, __signal_safe_memalign has a simple parseable -// header which we can use. +// +// Since 2.25: DTLS chunks are allocated with malloc. We could co-opt +// the malloc interceptor to keep track of the last allocation, similar +// to how we handle __libc_memalign; however, this adds some overhead +// (since malloc, unlike __libc_memalign, is commonly called), and +// requires care to avoid false negatives for LeakSanitizer. +// Instead, we rely on our internal allocators - which keep track of all +// its allocations - to determine if an address points to a malloc +// allocation. +// +// There exists a since-deprecated version of Google's internal glibc fork +// that used __signal_safe_memalign. DTLS_on_tls_get_addr relied on a +// heuristic check (is the allocation 16 bytes from the start of a page +// boundary?), which was sometimes erroneous: +// https://bugs.chromium.org/p/chromium/issues/detail?id=1275223#c15 +// Since that check has no practical use anymore, we have removed it. // //===----------------------------------------------------------------------===// diff --git a/compiler-rt/test/msan/dtls_test.c b/compiler-rt/test/msan/dtls_test.c index 45c8fd38bf5f6..3c384256147a0 100644 --- a/compiler-rt/test/msan/dtls_test.c +++ b/compiler-rt/test/msan/dtls_test.c @@ -12,10 +12,6 @@ // Reports use-of-uninitialized-value, not analyzed XFAIL: target={{.*netbsd.*}} - // This is known to be broken with glibc-2.27+ - // https://bugs.llvm.org/show_bug.cgi?id=37804 - XFAIL: glibc-2.27 - */ #ifndef BUILD_SO