Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[sanitizer] Add CHECK that static TLS info is ready #108684

Conversation

vitalybuka
Copy link
Collaborator

@vitalybuka vitalybuka commented Sep 14, 2024

There is possibility of
static_tls_begin is set and static_tls_end is not yet

The test reproduces the case.
Stack trace looks like this:

  • MsanThread::Init
  • SetThreadStackAndTls
  • GetThreadStackAndTls
  • GetThreadStackTopAndBottom
  • pthread_getattr_np
  • realloc
  • __sanitizer_malloc_hook
  • TLS access
  • ___interceptor___tls_get_addr
  • DTLS_on_tls_get_addr

The issue is that SetThreadStackAndTls implementation
stores tls_begin before GetThreadStackTopAndBottom,
and tls_end after. So we have partially initialized
state in DTLS_on_tls_get_addr.

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 14, 2024

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Vitaly Buka (vitalybuka)

Changes

There is possibility of
static_tls_begin is set and static_tls_end is not yet

The test reproduces the case.
Stack trace looks like this:
MsanThread::Init
SetThreadStackAndTls
GetThreadStackAndTls
GetThreadStackTopAndBottom
pthread_getattr_np
realloc
__sanitizer_malloc_hook
TLS access
___interceptor___tls_get_addr
DTLS_on_tls_get_addr

The issue is that SetThreadStackAndTls implementation
stores tls_begin before GetThreadStackTopAndBottom,
and tls_end after. So we have partially initialized
state in DTLS_on_tls_get_addr.


Full diff: https://github.com/llvm/llvm-project/pull/108684.diff

2 Files Affected:

  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_tls_get_addr.cpp (+1)
  • (added) compiler-rt/test/sanitizer_common/TestCases/Linux/tls_malloc_hook.c (+60)
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 087bd801b6e5ff..a17a14882d0e15 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_tls_get_addr.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_tls_get_addr.cpp
@@ -130,6 +130,7 @@ DTLS::DTV *DTLS_on_tls_get_addr(void *arg_void, void *res,
   DTLS::DTV *dtv = DTLS_Find(dso_id);
   if (!dtv || dtv->beg)
     return nullptr;
+  CHECK_LE(static_tls_begin, static_tls_end);
   uptr tls_size = 0;
   uptr tls_beg = reinterpret_cast<uptr>(res) - arg->offset - kDtvOffset;
   VReport(2,
diff --git a/compiler-rt/test/sanitizer_common/TestCases/Linux/tls_malloc_hook.c b/compiler-rt/test/sanitizer_common/TestCases/Linux/tls_malloc_hook.c
new file mode 100644
index 00000000000000..c582372ab9763d
--- /dev/null
+++ b/compiler-rt/test/sanitizer_common/TestCases/Linux/tls_malloc_hook.c
@@ -0,0 +1,60 @@
+// Test that we don't crash accessing DTLS from malloc hook.
+
+// RUN: %clang %s -o %t
+// RUN: %clang %s -DBUILD_SO -fPIC -o %t-so.so -shared
+// RUN: %run %t 2>&1 | FileCheck %s
+
+// REQUIRES: glibc
+
+// No allocator and hooks.
+// XFAIL: ubsan
+
+// FIXME: Crashes on CHECK.
+// XFAIL: asan && !i386-linux
+// XFAIL: msan && !i386-linux
+
+#ifndef BUILD_SO
+#  include <assert.h>
+#  include <dlfcn.h>
+#  include <pthread.h>
+#  include <stdio.h>
+#  include <stdlib.h>
+
+typedef long *(*get_t)();
+get_t GetTls;
+void *Thread(void *unused) { return GetTls(); }
+
+__thread long recursive_hook;
+
+// CHECK: __sanitizer_malloc_hook:
+void __sanitizer_malloc_hook(const volatile void *ptr, size_t sz)
+    __attribute__((disable_sanitizer_instrumentation)) {
+  ++recursive_hook;
+  if (recursive_hook == 1 && GetTls)
+    fprintf(stderr, "__sanitizer_malloc_hook: %p\n", GetTls());
+  --recursive_hook;
+}
+
+int main(int argc, char *argv[]) {
+  char path[4096];
+  snprintf(path, sizeof(path), "%s-so.so", argv[0]);
+  int i;
+
+  void *handle = dlopen(path, RTLD_LAZY);
+  if (!handle)
+    fprintf(stderr, "%s\n", dlerror());
+  assert(handle != 0);
+  GetTls = (get_t)dlsym(handle, "GetTls");
+  assert(dlerror() == 0);
+
+  pthread_t t;
+  pthread_create(&t, 0, Thread, 0);
+  pthread_join(t, 0);
+  pthread_create(&t, 0, Thread, 0);
+  pthread_join(t, 0);
+  return 0;
+}
+#else // BUILD_SO
+__thread long huge_thread_local_array[1 << 17];
+long *GetTls() { return &huge_thread_local_array[0]; }
+#endif

Created using spr 1.3.4

[skip ci]
Created using spr 1.3.4
@vitalybuka vitalybuka changed the base branch from users/vitalybuka/spr/main.sanitizer-add-check-that-static-tls-info-is-ready to main September 16, 2024 18:19
@vitalybuka vitalybuka merged commit 0ea0e3a into main Sep 16, 2024
7 of 9 checks passed
@vitalybuka vitalybuka deleted the users/vitalybuka/spr/sanitizer-add-check-that-static-tls-info-is-ready branch September 16, 2024 18:21
vitalybuka added a commit that referenced this pull request Sep 16, 2024
Fixes asan, msan crash on check added in #108684.
The #108684 includes reproducer of the issue.

Change interface of `GetThreadStackAndTls` to
set `tls_begin` and `tls_end` at the same time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants