[PATCH] D30818: [lsan] Don't handle DTLS of thread under destruction

[Maxim Ostapenko via Phabricator via llvm-commits]

m.ostapenko updated this revision to Diff 91522.
m.ostapenko added a comment.

Updating according to Vitaly's nits.

When testing new patch, I've encountered another error:

  ==7509==Processing thread 7507.
  ==7509==Could not get registers from thread 7507 (errno 3).
  ==7509==Unable to get registers from thread 7507.
  ==7509==Stack at 0x7f0c71bfa000-0x7f0c723ec980 (SP = 0x7f0c71bfa000).
  Tracer caught signal 11: addr=0x7f0c71bfa000 pc=0x423980 sp=0x7f0c41b99da0
  ==7509==Could not detach from thread 7507 (errno 3).
  ==25753==LeakSanitizer has encountered a fatal error.
  ==25753==HINT: For debugging, try setting environment variable LSAN_OPTIONS=verbosity=1:log_threads=1
  ==25753==HINT: LeakSanitizer does not work under ptrace (strace, gdb, etc)

This happens because the stack memory of (destroyed) thread can be already unmapped at point of analysis thus accessing it later would lead to segfault.
I've introduced a new flag (off by default) to mitigate the issue.

Regarding to testcase, I'm afraid we'll need something like testcase from GitHub issue (infinite program with lots of threads creation/destruction) to trigger the issue, that might be too intrusive/flaky for buildbots. If it's acceptable to add such a test, I can add it for sure.


Repository:
  rL LLVM

https://reviews.llvm.org/D30818

Files:
  lib/lsan/lsan_common.cc
  lib/lsan/lsan_flags.inc
  lib/sanitizer_common/sanitizer_tls_get_addr.cc
  lib/sanitizer_common/sanitizer_tls_get_addr.h


Index: lib/sanitizer_common/sanitizer_tls_get_addr.h
===================================================================
--- lib/sanitizer_common/sanitizer_tls_get_addr.h
+++ lib/sanitizer_common/sanitizer_tls_get_addr.h
@@ -55,6 +55,8 @@
 void DTLS_on_libc_memalign(void *ptr, uptr size);
 DTLS *DTLS_Get();
 void DTLS_Destroy();  // Make sure to call this before the thread is destroyed.
+// Returns true if DTLS of suspended thread is in destruction process.
+bool DTLSInDestruction(DTLS *dtls);
 
 }  // namespace __sanitizer
 
Index: lib/sanitizer_common/sanitizer_tls_get_addr.cc
===================================================================
--- lib/sanitizer_common/sanitizer_tls_get_addr.cc
+++ lib/sanitizer_common/sanitizer_tls_get_addr.cc
@@ -136,11 +136,17 @@
 
 DTLS *DTLS_Get() { return &dtls; }
 
+bool DTLSInDestruction(DTLS *dtls) {
+  return dtls->dtv_size == kDestroyedThread;
+}
+
 #else
 void DTLS_on_libc_memalign(void *ptr, uptr size) {}
 DTLS::DTV *DTLS_on_tls_get_addr(void *arg, void *res) { return 0; }
 DTLS *DTLS_Get() { return 0; }
 void DTLS_Destroy() {}
+bool DTLSInDestruction(DTLS *dtls) { UNREACHABLE(); }
+
 #endif  // SANITIZER_INTERCEPT_TLS_GET_ADDR
 
 }  // namespace __sanitizer
Index: lib/lsan/lsan_flags.inc
===================================================================
--- lib/lsan/lsan_flags.inc
+++ lib/lsan/lsan_flags.inc
@@ -45,3 +45,6 @@
 LSAN_FLAG(bool, log_pointers, false, "Debug logging")
 LSAN_FLAG(bool, log_threads, false, "Debug logging")
 LSAN_FLAG(const char *, suppressions, "", "Suppressions file name.")
+LSAN_FLAG(bool, consider_entire_stack_reachable, true,
+          "Consider entire stack to be reachable if we unable to retrive SP "
+          "from suspended thread.")
Index: lib/lsan/lsan_common.cc
===================================================================
--- lib/lsan/lsan_common.cc
+++ lib/lsan/lsan_common.cc
@@ -204,9 +204,15 @@
     bool have_registers =
         (suspended_threads.GetRegistersAndSP(i, registers.data(), &sp) == 0);
     if (!have_registers) {
-      Report("Unable to get registers from thread %d.\n");
-      // If unable to get SP, consider the entire stack to be reachable.
-      sp = stack_begin;
+      Report("Unable to get registers from thread %d.\n", os_id);
+      // If unable to get SP, consider the entire stack to be reachable
+      // depending on consider_entire_stack_reachable flag. We cannot do this
+      // unconditionally because stack memory can be already unmapped at this
+      // point.
+      if (flags()->consider_entire_stack_reachable)
+        sp = stack_begin;
+      else
+        continue;
     }
 
     if (flags()->use_registers && have_registers)
@@ -253,7 +259,7 @@
         if (tls_end > cache_end)
           ScanRangeForPointers(cache_end, tls_end, frontier, "TLS", kReachable);
       }
-      if (dtls) {
+      if (dtls && !DTLSInDestruction(dtls)) {
         for (uptr j = 0; j < dtls->dtv_size; ++j) {
           uptr dtls_beg = dtls->dtv[j].beg;
           uptr dtls_end = dtls_beg + dtls->dtv[j].size;
@@ -263,6 +269,10 @@
                                  kReachable);
           }
         }
+      } else {
+        // We are handling a thread with DTLS under destruction. Log about
+        // this and continue.
+        LOG_THREADS("Thread %d has DTLS under destruction.\n", os_id);
       }
     }
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D30818.91522.patch
Type: text/x-patch
Size: 3378 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170313/5a1521ee/attachment.bin>