[sanitizer] Fix asserts in asan and tsan in pthread interceptors. #75394

goussepi · 2023-12-13T22:22:59Z

Calling one of pthread join/detach interceptor on an already joined/detached thread causes asserts such as:

AddressSanitizer: CHECK failed: sanitizer_thread_arg_retval.cpp:56 "((t)) != (0)" (0x0, 0x0) (tid=1236094)
#0 0x555555634f8b in __asan::CheckUnwind() compiler-rt/lib/asan/asan_rtl.cpp:69:3
#1 0x55555564e06e in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) compiler-rt/lib/sanitizer_common/sanitizer_termination.cpp:86:24
#2 0x5555556491df in __sanitizer::ThreadArgRetval::BeforeJoin(unsigned long) const compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.cpp:56:3
#3 0x5555556198ed in Join<___interceptor_pthread_tryjoin_np(void*, void**)::<lambda()> > compiler-rt/lib/asan/../sanitizer_common/sanitizer_thread_arg_retval.h:74:26
#4 0x5555556198ed in pthread_tryjoin_np compiler-rt/lib/asan/asan_interceptors.cpp:311:29

The assert are replaced by error codes.

llvmbot · 2023-12-13T22:23:16Z

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

Author: None (goussepi)

Changes

Calling one of pthread join/detach interceptor on an already joined/detached thread causes asserts such as:

AddressSanitizer: CHECK failed: sanitizer_thread_arg_retval.cpp:56 "((t)) != (0)" (0x0, 0x0) (tid=1236094)
#0 0x555555634f8b in __asan::CheckUnwind() compiler-rt/lib/asan/asan_rtl.cpp:69:3
#1 0x55555564e06e in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) compiler-rt/lib/sanitizer_common/sanitizer_termination.cpp:86:24
#2 0x5555556491df in __sanitizer::ThreadArgRetval::BeforeJoin(unsigned long) const compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.cpp:56:3
#3 0x5555556198ed in Join<___interceptor_pthread_tryjoin_np(void*, void**)::<lambda()> > compiler-rt/lib/asan/../sanitizer_common/sanitizer_thread_arg_retval.h:74:26
#4 0x5555556198ed in pthread_tryjoin_np compiler-rt/lib/asan/asan_interceptors.cpp:311:29

The assert are replaced by error codes.

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

4 Files Affected:

(modified) compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.cpp (+2-1)
(modified) compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.cpp (+2-1)
(modified) compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp (+2-2)
(modified) compiler-rt/test/sanitizer_common/TestCases/Linux/pthread_join.cpp (+9)

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.cpp
index bddb2852140854..5edf6f1764f966 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.cpp
@@ -53,7 +53,8 @@ void ThreadArgRetval::Finish(uptr thread, void* retval) {
 u32 ThreadArgRetval::BeforeJoin(uptr thread) const {
   __sanitizer::Lock lock(&mtx_);
   auto t = data_.find(thread);
-  CHECK(t);
+  if (!t)
+    return 0;
   CHECK(!t->second.detached);
   return t->second.gen;
 }
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.cpp
index 741e0731c41559..12d36277589626 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.cpp
@@ -345,7 +345,8 @@ u32 ThreadRegistry::ConsumeThreadUserId(uptr user_id) {
   ThreadRegistryLock l(this);
   u32 tid;
   auto *t = live_.find(user_id);
-  CHECK(t);
+  if (!t)
+    return kInvalidTid;
   tid = t->second;
   live_.erase(t);
   auto *tctx = threads_[tid];
diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
index 80f86ca98ed9cd..bef0aa373119fa 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
@@ -1118,7 +1118,7 @@ TSAN_INTERCEPTOR(int, pthread_tryjoin_np, void *th, void **ret) {
   ThreadIgnoreEnd(thr);
   if (res == 0)
     ThreadJoin(thr, pc, tid);
-  else
+  else if (tid != kInvalidTid)
     ThreadNotJoined(thr, pc, tid, (uptr)th);
   return res;
 }
@@ -1132,7 +1132,7 @@ TSAN_INTERCEPTOR(int, pthread_timedjoin_np, void *th, void **ret,
   ThreadIgnoreEnd(thr);
   if (res == 0)
     ThreadJoin(thr, pc, tid);
-  else
+  else if (tid != kInvalidTid)
     ThreadNotJoined(thr, pc, tid, (uptr)th);
   return res;
 }
diff --git a/compiler-rt/test/sanitizer_common/TestCases/Linux/pthread_join.cpp b/compiler-rt/test/sanitizer_common/TestCases/Linux/pthread_join.cpp
index 212a28dd3985bb..dd70f12fbbef6c 100644
--- a/compiler-rt/test/sanitizer_common/TestCases/Linux/pthread_join.cpp
+++ b/compiler-rt/test/sanitizer_common/TestCases/Linux/pthread_join.cpp
@@ -4,6 +4,10 @@
 // FIXME: Crashes on some bots in pthread_exit.
 // RUN: %run %t %if tsan %{ 0 %} %else %{ 1 %}
 
+// Check interceptors' return codes are the same without sanitizers.
+// RUN: %clangxx -pthread -fno-sanitize=all %s -o %t
+// RUN: %run %t 0
+
 // REQUIRES: glibc
 
 #include <assert.h>
@@ -12,6 +16,7 @@
 #include <stdint.h>
 #include <stdlib.h>
 #include <unistd.h>
+#include <errno.h>
 
 bool use_exit;
 static void *fn(void *args) {
@@ -36,12 +41,14 @@ int main(int argc, char **argv) {
   assert(!pthread_attr_destroy(&attr));
 
   assert(!pthread_detach(thread[0]));
+  assert(pthread_detach(thread[0]) == EINVAL);
 
   {
     void *res;
     while (pthread_tryjoin_np(thread[1], &res))
       sleep(1);
     assert(~(uintptr_t)res == 1001);
+    assert(pthread_tryjoin_np(thread[1], &res) == EBUSY);
   }
 
   {
@@ -50,12 +57,14 @@ int main(int argc, char **argv) {
     while (pthread_timedjoin_np(thread[2], &res, &tm))
       sleep(1);
     assert(~(uintptr_t)res == 1002);
+    assert(pthread_timedjoin_np(thread[2], &res, &tm) == ESRCH);
   }
 
   {
     void *res;
     assert(!pthread_join(thread[3], &res));
     assert(~(uintptr_t)res == 1003);
+    assert(pthread_join(thread[3], &res) == ESRCH);
   }
 
   return 0;

github-actions · 2023-12-13T22:25:55Z

✅ With the latest revision this PR passed the C/C++ code formatter.

vitalybuka · 2023-12-14T00:06:21Z

@dvyukov had strong opinion on that.
I would probably prefer sanitizer crash with report with explanation.
But I don't like silently return error here as well.

From pthread_detach manual

       Attempting to detach an already detached thread results in
       unspecified behavior.

There is no promise to return particular error on the second detach. User code should not do that.

However sanitizers can get into inconsistent state, which will be hard to debug.

dvyukov · 2023-12-14T06:34:14Z

Yes, I wonder what's the pattern in user code that triggers this and is not a bug.
Error codes are frequently ignored.
Perhaps print a better explanatory message before crashing.

goussepi · 2023-12-14T14:25:18Z

Thanks @vitalybuka @dvyukov for the comments.
Yes, I agree no need to check for invalid pthread_detach calls’ return value, I have removed it from the test.
Regarding the pthread join apis, what about not asserting unless running Tsan (cf test update) ?
Many thanks!
Pierre

vitalybuka · 2023-12-18T18:43:15Z

compiler-rt/test/sanitizer_common/TestCases/Linux/pthread_join.cpp

@@ -4,10 +4,15 @@
 // FIXME: Crashes on some bots in pthread_exit.
 // RUN: %run %t %if tsan %{ 0 %} %else %{ 1 %}

+// Check interceptors' return codes are the same without sanitizers.


I don't see value in this test
-fno-sanitize=all removes runtime, we don't have interceptors.

I meant it as a sanity check to double check the interceptors returns the same error code as the intercepted functions.
Not strictly required so happy to remove.

vitalybuka · 2023-12-18T19:14:24Z

compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.cpp

@@ -53,7 +53,8 @@ void ThreadArgRetval::Finish(uptr thread, void* retval) {
 u32 ThreadArgRetval::BeforeJoin(uptr thread) const {
  __sanitizer::Lock lock(&mtx_);
  auto t = data_.find(thread);
-  CHECK(t);
+  if (!t)
+    return 0;
  CHECK(!t->second.detached);


we still have the case when joined thread is reused by another 'detached' thread and we will hit the CHECK below

A yes of course, will fix thanks!

vitalybuka · 2023-12-18T19:27:32Z

It's not critical for this component to mismatch join

However, it could be nice to see example why keeping the bug in the program is needed?
Obvious danger of this is thread reuse, and you are joining alive but wrong thread.
Sanitizer does not detect that, but if you hit this CHECK, it's possible.

Maybe rather introduce detect_invalid_join into sanitizer_flags.inc, set it true by default, and for let users disable it?
Would you like to do that? If not, please add TODO near the current CHECK

goussepi · 2023-12-19T14:12:50Z

It's not critical for this component to mismatch join

However, it could be nice to see example why keeping the bug in the program is needed? Obvious danger of this is thread reuse, and you are joining alive but wrong thread. Sanitizer does not detect that, but if you hit this CHECK, it's possible.

Maybe rather introduce detect_invalid_join into sanitizer_flags.inc, set it true by default, and for let users disable it? Would you like to do that? If not, please add TODO near the current CHECK

The error report I got was for pthread_join being called unconditionally inside a destructor.
I agree they are bugs but users see a crash and it can end up being reported as a sanitizer issue.
Adding a flag such as detect_invalid_join sounds good, and using Report() + Die() rather than a CHECK ?

Many thanks!

Pierre

vitalybuka · 2023-12-19T20:54:12Z

It's not critical for this component to mismatch join
However, it could be nice to see example why keeping the bug in the program is needed? Obvious danger of this is thread reuse, and you are joining alive but wrong thread. Sanitizer does not detect that, but if you hit this CHECK, it's possible.
Maybe rather introduce detect_invalid_join into sanitizer_flags.inc, set it true by default, and for let users disable it? Would you like to do that? If not, please add TODO near the current CHECK

The error report I got was for pthread_join being called unconditionally inside a destructor. I agree they are bugs but users see a crash and it can end up being reported as a sanitizer issue. Adding a flag such as detect_invalid_join sounds good, and using Report() + Die() rather than a CHECK ?

Many thanks!

Pierre

Yes, we need Report() + Die().

goussepi · 2024-01-09T14:03:29Z

ping!

vitalybuka · 2024-01-10T08:12:55Z

ping!

There is "re-request" review, otherwise it does not show up in searches like this https://github.com/llvm/llvm-project/pulls/review-requested/@me

vitalybuka · 2024-01-10T08:21:54Z

compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.cpp

-  CHECK(t);
-  CHECK(!t->second.detached);
-  return t->second.gen;
+  bool detect_invalid_join = common_flags()->detect_invalid_join;


if (t && !t->second.detached) { return t->second.gen; } if (!common_flags()->detect_invalid_join) return kInvalidGen; const char* const reason = "unknown"; if (!t) { reason = "already joined"; } else if (t->second.detached) { reason = "detached"; } Report("ERROR: %s: Joining %s thread, aborting.\n", SanitizerToolName, reason); Die();

vitalybuka · 2024-01-10T08:26:15Z

compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.cpp

 }

 void ThreadArgRetval::AfterJoin(uptr thread, u32 gen) {
  __sanitizer::Lock lock(&mtx_);
  auto t = data_.find(thread);
-  if (!t || gen != t->second.gen) {
-    // Thread was reused and erased by any other event.
+  if (!t || gen != t->second.gen || gen == kInvalidGen) {


we should not have t->second.gen = kInvalidGen,

to make sure skip invalid here .gen = gen_++;

vitalybuka

LGTM
Thank you!

vitalybuka · 2024-01-10T22:49:15Z

compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.cpp

@@ -23,6 +23,9 @@ void ThreadArgRetval::CreateLocked(uptr thread, bool detached,
  Data& t = data_[thread];
  t = {};
  t.gen = gen_++;
+  static_assert(sizeof(gen_) == sizeof(u32) && kInvalidGen == UINT32_MAX);
+  if (gen_ == kInvalidGen)


I guess I missed if (gen_ == kInvalidGen) = 0,
so we don't need the one introduced with b96deb8

vitalybuka · 2024-01-10T22:51:16Z

I believe "member" can merge?
If not, please re-request, and I'll merge

Calling one of pthread join/detach interceptor on an already joined/detached thread causes asserts such as: AddressSanitizer: CHECK failed: sanitizer_thread_arg_retval.cpp:56 "((t)) != (0)" (0x0, 0x0) (tid=1236094) #0 0x555555634f8b in __asan::CheckUnwind() compiler-rt/lib/asan/asan_rtl.cpp:69:3 llvm#1 0x55555564e06e in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) compiler-rt/lib/sanitizer_common/sanitizer_termination.cpp:86:24 llvm#2 0x5555556491df in __sanitizer::ThreadArgRetval::BeforeJoin(unsigned long) const compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.cpp:56:3 llvm#3 0x5555556198ed in Join<___interceptor_pthread_tryjoin_np(void*, void**)::<lambda()> > compiler-rt/lib/asan/../sanitizer_common/sanitizer_thread_arg_retval.h:74:26 llvm#4 0x5555556198ed in pthread_tryjoin_np compiler-rt/lib/asan/asan_interceptors.cpp:311:29 This change does 2 things: - Introduce a detect_invalid_join common flags - Replace CHECK assert by Report() and Die()

…vm#75394) Calling one of pthread join/detach interceptor on an already joined/detached thread causes asserts such as: AddressSanitizer: CHECK failed: sanitizer_thread_arg_retval.cpp:56 "((t)) != (0)" (0x0, 0x0) (tid=1236094) #0 0x555555634f8b in __asan::CheckUnwind() compiler-rt/lib/asan/asan_rtl.cpp:69:3 llvm#1 0x55555564e06e in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) compiler-rt/lib/sanitizer_common/sanitizer_termination.cpp:86:24 llvm#2 0x5555556491df in __sanitizer::ThreadArgRetval::BeforeJoin(unsigned long) const compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.cpp:56:3 llvm#3 0x5555556198ed in Join<___interceptor_pthread_tryjoin_np(void*, void**)::<lambda()> > compiler-rt/lib/asan/../sanitizer_common/sanitizer_thread_arg_retval.h:74:26 llvm#4 0x5555556198ed in pthread_tryjoin_np compiler-rt/lib/asan/asan_interceptors.cpp:311:29 The assert are replaced by error codes.

goussepi added compiler-rt compiler-rt:sanitizer labels Dec 13, 2023

goussepi requested review from dvyukov and vitalybuka December 13, 2023 22:23

vitalybuka reviewed Dec 18, 2023

View reviewed changes

vitalybuka closed this Dec 18, 2023

vitalybuka reopened this Dec 18, 2023

vitalybuka reviewed Jan 10, 2024

View reviewed changes

goussepi requested a review from vitalybuka January 10, 2024 16:29

vitalybuka approved these changes Jan 10, 2024

View reviewed changes

goussepi force-pushed the pthread_join-interceptors-crash-fix branch from 5f3b82b to 4e031a9 Compare January 11, 2024 10:43

goussepi merged commit 5e40661 into llvm:main Jan 11, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sanitizer] Fix asserts in asan and tsan in pthread interceptors. #75394

[sanitizer] Fix asserts in asan and tsan in pthread interceptors. #75394

goussepi commented Dec 13, 2023

llvmbot commented Dec 13, 2023

github-actions bot commented Dec 13, 2023 •

edited

Loading

vitalybuka commented Dec 14, 2023

dvyukov commented Dec 14, 2023

goussepi commented Dec 14, 2023

vitalybuka Dec 18, 2023

goussepi Dec 19, 2023

vitalybuka Dec 18, 2023

goussepi Dec 19, 2023

vitalybuka commented Dec 18, 2023

goussepi commented Dec 19, 2023

vitalybuka commented Dec 19, 2023

goussepi commented Jan 9, 2024

vitalybuka commented Jan 10, 2024

vitalybuka Jan 10, 2024

vitalybuka Jan 10, 2024

vitalybuka left a comment

vitalybuka Jan 10, 2024

vitalybuka commented Jan 10, 2024

[sanitizer] Fix asserts in asan and tsan in pthread interceptors. #75394

[sanitizer] Fix asserts in asan and tsan in pthread interceptors. #75394

Conversation

goussepi commented Dec 13, 2023

llvmbot commented Dec 13, 2023

github-actions bot commented Dec 13, 2023 • edited Loading

vitalybuka commented Dec 14, 2023

dvyukov commented Dec 14, 2023

goussepi commented Dec 14, 2023

vitalybuka Dec 18, 2023

Choose a reason for hiding this comment

goussepi Dec 19, 2023

Choose a reason for hiding this comment

vitalybuka Dec 18, 2023

Choose a reason for hiding this comment

goussepi Dec 19, 2023

Choose a reason for hiding this comment

vitalybuka commented Dec 18, 2023

goussepi commented Dec 19, 2023

vitalybuka commented Dec 19, 2023

goussepi commented Jan 9, 2024

vitalybuka commented Jan 10, 2024

vitalybuka Jan 10, 2024

Choose a reason for hiding this comment

vitalybuka Jan 10, 2024

Choose a reason for hiding this comment

vitalybuka left a comment

Choose a reason for hiding this comment

vitalybuka Jan 10, 2024

Choose a reason for hiding this comment

vitalybuka commented Jan 10, 2024

github-actions bot commented Dec 13, 2023 •

edited

Loading