[sanitizer] Don't miss threads by ThreadSuspender

[vitalybuka]

Summary

Enumerating /proc/<pid>/task/ dir Linux may stop if thread is dead. In this case
we miss some threads and report false memory leaks.
To solve this issue we repeat enumeration if the last thread is dead.
Do detect dead threads same way as proc_task_readdir we use
/proc/<pid>/task/<tid>/status.

Similarly it also ends enumeration of if proc_fill_cache fails, but in this case
Linux sets inode to 1 (Bad block).

[eugenis]

What if there is no short read and the entire list is read in one syscall - does that mean that we've got a consistent thread list for some point of time in the past? If so, then iterating until the list stops changing and ignoring any attempts that ended in short reads should work.

[vitalybuka]

In D46517#1090037, @eugenis wrote:

What if there is no short read and the entire list is read in one syscall - does that mean that we've got a consistent thread list for some point of time in the past? If so, then iterating until the list stops changing and ignoring any attempts that ended in short reads should work.

I already tried that. It's possible to have no short read but still small list. Weird that even /proc/<pid>/status:Threads contains small value which match this incomplete list.

[vitalybuka]

here I explain what I see WITHOUT this patch:
I have small program which creates 30 threads, which exit almost immediately. And another thread which runs __lsan_do_recoverable_leak_check().

Here the last successful __lsan_do_recoverable_leak_check

SuspendAllThreads
 ThreadLister::ListThreads
  internal_getdents: 840
   /proc/6563/task/6563
   /proc/6563/task/6564
   /proc/6563/task/6565
   /proc/6563/task/6566
   /proc/6563/task/6929
   /proc/6563/task/6930
   /proc/6563/task/6931
   /proc/6563/task/6932
   /proc/6563/task/6933
   /proc/6563/task/6934
   /proc/6563/task/6935
   /proc/6563/task/6936
   /proc/6563/task/6939
   /proc/6563/task/6940
   /proc/6563/task/6941
   /proc/6563/task/6942
   /proc/6563/task/6943
   /proc/6563/task/6944
   /proc/6563/task/6945
   /proc/6563/task/6946
   /proc/6563/task/6949
   /proc/6563/task/6950
   /proc/6563/task/6951
   /proc/6563/task/6952
   /proc/6563/task/6955
   /proc/6563/task/6956
   /proc/6563/task/6957
   /proc/6563/task/6958
   /proc/6563/task/6959
   /proc/6563/task/6960
   /proc/6563/task/6961
   /proc/6563/task/6966
   /proc/6563/task/6968
  internal_getdents: 0
 ThreadLister::ListThreads
  internal_getdents: 840
   /proc/6563/task/6563
   /proc/6563/task/6564
   /proc/6563/task/6565
   /proc/6563/task/6566
   /proc/6563/task/6929
   /proc/6563/task/6930
   /proc/6563/task/6931
   /proc/6563/task/6932
   /proc/6563/task/6933
   /proc/6563/task/6934
   /proc/6563/task/6935
   /proc/6563/task/6936
   /proc/6563/task/6939
   /proc/6563/task/6940
   /proc/6563/task/6941
   /proc/6563/task/6942
   /proc/6563/task/6943
   /proc/6563/task/6944
   /proc/6563/task/6945
   /proc/6563/task/6946
   /proc/6563/task/6949
   /proc/6563/task/6950
   /proc/6563/task/6951
   /proc/6563/task/6952
   /proc/6563/task/6955
   /proc/6563/task/6956
   /proc/6563/task/6957
   /proc/6563/task/6958
   /proc/6563/task/6959
   /proc/6563/task/6960
   /proc/6563/task/6961
   /proc/6563/task/6966
   /proc/6563/task/6968
  internal_getdents: 0

Leak check iterated list twice with same list of thread and suspended following:

Suspended
 suspended: 6563
 suspended: 6564
 suspended: 6565
 suspended: 6566
 suspended: 6929
 suspended: 6930
 suspended: 6931
 suspended: 6932
 suspended: 6933
 suspended: 6934
 suspended: 6935
 suspended: 6936
 suspended: 6939
 suspended: 6940
 suspended: 6941
 suspended: 6942
 suspended: 6943
 suspended: 6944
 suspended: 6945
 suspended: 6946
 suspended: 6949
 suspended: 6950
 suspended: 6951
 suspended: 6952
 suspended: 6955
 suspended: 6956
 suspended: 6957
 suspended: 6958
 suspended: 6959
 suspended: 6960
 suspended: 6961
 suspended: 6966
 suspended: 6968

Note threads 6958 and 6968.

The next call to __lsan_do_recoverable_leak_check()

SuspendAllThreads
 ThreadLister::ListThreads
  internal_getdents: 240
   /proc/6563/task/6563
   /proc/6563/task/6564
   /proc/6563/task/6565
   /proc/6563/task/6566
   /proc/6563/task/6930
   /proc/6563/task/6942
   /proc/6563/task/6944
   /proc/6563/task/6950
  internal_getdents: 0

Some threads are gone including 6958 and 6968.
The second internal_getdents returned 0 so it looks like a complete list.

Do one more iterations:

 ThreadLister::ListThreads
  internal_getdents: 168
   /proc/6563/task/6563
   /proc/6563/task/6564
   /proc/6563/task/6565
   /proc/6563/task/6566
   /proc/6563/task/6958
  internal_getdents: 0
Suspended
 suspended: 6563
 suspended: 6564
 suspended: 6565
 suspended: 6566

Thread 6958 is exiting so we failed to suspend it and we return from SuspendAllThreads.

However if we call ThreadLister one more time (just for debugging, without actually suspending):

ThreadLister::ListThreads
 internal_getdents: 168
  /proc/6563/task/6563
  /proc/6563/task/6564
  /proc/6563/task/6565
  /proc/6563/task/6566
  /proc/6563/task/6968
 internal_getdents: 0

6968 is back and we didn't suspend it.

Then we proceed leak checking and get:

==6563==ERROR: LeakSanitizer: detected memory leaks
Objects leaked above:
0x613000072500 (352 bytes)

If we ignore the leak and continue, next call to __lsan_do_recoverable_leak_check will see and suspend 6968 thread again.

SuspendAllThreads
 ThreadLister::ListThreads
  internal_getdents: 168
   /proc/6563/task/6563
   /proc/6563/task/6564
   /proc/6563/task/6565
   /proc/6563/task/6566
   /proc/6563/task/6968
  internal_getdents: 0
 ThreadLister::ListThreads
  internal_getdents: 168
   /proc/6563/task/6563
   /proc/6563/task/6564
   /proc/6563/task/6565
   /proc/6563/task/6566
   /proc/6563/task/6968
  internal_getdents: 0
Suspended
 suspended: 6563
 suspended: 6564
 suspended: 6565
 suspended: 6566
 suspended: 6968

So obviously increasing iteration highly reduce probability of missing threads but not guaranty correctness.
Reason is probably this place https://github.com/torvalds/linux/blob/master/fs/proc/base.c#L3562
When it hits dying thread, it exits as list is complete.

[vitalybuka]

Looks like I can assume that kernel stops enumerating on the first !pid_alive()
So I can assume that if the last thread in the returned list is !pid_alive() I need one more iteration.
I have no access to pid_alive() but according kernel source PPid of such thread is going to be set to 0.

Seems this reliably works. I will update the patch.

[vitalybuka]

different approach

[vitalybuka]

remove unrelated changes

[eugenis]

Nice!

compiler-rt/lib/sanitizer_common/sanitizer_file.cc

169 ↗

(On Diff #145739)

I don't like this code duplication at all.

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

973–988

Would it be better to resize the buffer and start from scratch until we get the entire directory in one go? Otherwise we can still miss a thread that was added before the current directory cursor, I guess - or is that impossible?

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

229–230

Is this because you want to suspend threads even when you've got an incomplete response? If would be cleaner to just loop while(Incomplete) ListThreads() and then handle the other two return codes.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_file.cc

169 ↗

(On Diff #145739)

Let me resolve this a separate patch.

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

973–988

If it's not asan (thread registry is blocked there) threads may continue create new threads. So better to stop whatever is known.

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

229–230

It would be cleaner, but not sure about probability of issues.
Also if you just reread immediatly, it will likely return same list because the dying thread is still there. Suspending threads we give some time to kernel to remove it from list.

[eugenis]

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

973–988

Yes, if the read stopped on a dead/dying thread.
No, if it stopped because it ran out of buffer space.
Basically, we want the final iteration to succeed in a single system call, without any dead threads, and without any threads that have not been stopped on previous iterations. Otherwise we don't know if we've stopped everything.

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

229–230

Agreed, it can't hurt to stop what we can as soon as we can.

[vitalybuka]

try to avoid short read

[vitalybuka]

CHECK

[krytarowski]

While there, listing threads on NetBSD has to be done differently. If I'm looking correctly this code will be used for this OS too... and fail.

[krytarowski]

But we can skip it, I will address it in future.

[eugenis]

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

935

s/termination/terminated

942

I can't parse this comment.
s/with/we, s/about/above ?

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

222

Maybe rename to smth like "bool retry" ? Otherwise it is not clear why an incomplete list results in added_threads = true.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

942

I restricted the code and comments https://reviews.llvm.org/rL331953

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

942

restricted -> restructed

compiler-rt/lib/sanitizer_common/sanitizer_linux.h

 #endif
 #endif  // SANITIZER_LINUX
 
 // This class reads thread IDs from /proc/<pid>/task using only syscalls.
 class ThreadLister {
  public:
   explicit ThreadLister(int pid);
   ~ThreadLister();
-  bool ListThreads(InternalMmapVector<int> *threads);
+  enum Result {
+    Error,
+    Incomplete,
+    Ok,
+  };
+  Result ListThreads(InternalMmapVector<int> *threads);
 
  private:
+  bool IsAlive(int tid);
+
   int pid_;
   int descriptor_ = -1;
   InternalMmapVector<char> buffer_;
 };
 
 // Exposed for testing.
 uptr ThreadDescriptorSize();
 uptr ThreadSelf();

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

bool internal_sigismember(__sanitizer_sigset_t *set, int signum) {
   const uptr bit = signum % (sizeof(k_set->sig[0]) * 8);
   return k_set->sig[idx] & (1 << bit);
 }
 #endif  // SANITIZER_LINUX
 #endif // !SANITIZER_SOLARIS
 
 // ThreadLister implementation.
 ThreadLister::ThreadLister(int pid) : pid_(pid), buffer_(4096) {
-  buffer_.resize(buffer_.capacity());
   char task_directory_path[80];
   internal_snprintf(task_directory_path, sizeof(task_directory_path),
                     "/proc/%d/task/", pid);
   descriptor_ = internal_open(task_directory_path, O_RDONLY | O_DIRECTORY);
   if (internal_iserror(descriptor_)) {
     Report("Can't open /proc/%d/task for reading.\n", pid);
   }
 }
 
-bool ThreadLister::ListThreads(InternalMmapVector<int> *threads) {
+ThreadLister::Result ThreadLister::ListThreads(
+    InternalMmapVector<int> *threads) {
   if (internal_iserror(descriptor_))
-    return false;
+    return Error;
   internal_lseek(descriptor_, 0, SEEK_SET);
   threads->clear();
 
-  while (uptr read = internal_getdents(descriptor_,
-                                       (struct linux_dirent *)buffer_.data(),
-                                       buffer_.size())) {
+  uptr read_total = 0;
+  for (int i = 0;; ++i) {
+    // Resize to max capacity if it was downsized by IsAlive.
+    buffer_.resize(buffer_.capacity());
+    uptr read = internal_getdents(
+        descriptor_, (struct linux_dirent *)buffer_.data(), buffer_.size());
+    if (!read) {
+      if (read_total > buffer_.size() - 1024) {
+        // If proc_task_readdir returned because buffer was not enough and it
+        // stopped on thread which is termination it may not recover position.
+        // In this case we lose the rest and may be unable to recognize this as
+        // a short read.
+        buffer_.resize(buffer_.size() * 2);
+        return Incomplete;
+      }
+      if (i > 1) {
+        // With probably should not get here with check about.
+        // We can have short read because of small buffer (handled above),
+        // and because kernel tried to emit dying process (handled as Inode 1).
+        return Incomplete;
+      }
+      if (!threads->empty()) {
+        // Linux always stop enumeration after returning thread !pid_alive
+        // thread. See next_tid implementation in Linux.
+        if (!IsAlive(threads->back()))
+          return Incomplete;
+      }
+      return Ok;
+    }
     if (internal_iserror(read)) {
       Report("Can't read directory entries from /proc/%d/task.\n", pid_);
-      return false;
+      return Error;
     }
+    read_total += read;
 
     for (uptr begin = (uptr)buffer_.data(), end = begin + read; begin < end;) {
       struct linux_dirent *entry = (struct linux_dirent *)begin;
       begin += entry->d_reclen;
-      if (entry->d_ino && *entry->d_name >= '0' && *entry->d_name <= '9') {
-        // Found a valid tid.
+      if (entry->d_ino == 1) {
+        // Inode 1 is for bad blocks and also can be a reason for early return.
+        // Should be emitted if kernel started tried to output dying thread.
+        // See proc_task_readdir implementation in Linux.
+        return Incomplete;
+      }
+      if (entry->d_ino && *entry->d_name >= '0' && *entry->d_name <= '9')
         threads->push_back(internal_atoll(entry->d_name));
-      }
-    }
-  }
-  return true;
+
+bool ThreadLister::IsAlive(int tid) {
+  // /proc/%d/task/%d/status uses same call to detect alive threads as
+  // proc_task_readdir. See task_state implementation in Linux.
+  char path[80];
+  internal_snprintf(path, sizeof(path), "/proc/%d/task/%d/status", pid_, tid);
+  if (!ReadFileToVector(path, &buffer_) || buffer_.empty())
+    return false;
+  buffer_.push_back(0);
+  static const char kPrefix[] = "\nPPid:";
+  const char *field = internal_strstr(buffer_.data(), kPrefix);
+  if (!field)
+    return false;
+  field += internal_strlen(kPrefix);
+  return (int)internal_atoll(field) != 0;
 }
 
 ThreadLister::~ThreadLister() {
   if (!internal_iserror(descriptor_))
     internal_close(descriptor_);
 }
 
 #if SANITIZER_WORDSIZE == 32

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

void ThreadSuspender::KillAllThreads() {
   for (uptr i = 0; i < suspended_threads_list_.ThreadCount(); i++)
     internal_ptrace(PTRACE_KILL, suspended_threads_list_.GetThreadID(i),
                     nullptr, nullptr);
 }
 
 bool ThreadSuspender::SuspendAllThreads() {
   ThreadLister thread_lister(pid_);
   bool added_threads;
-  bool first_iteration = true;
   InternalMmapVector<int> threads;
   threads.reserve(128);
   do {
-    // Run through the directory entries once.
     added_threads = false;
-    if (!thread_lister.ListThreads(&threads)) {
+    switch (thread_lister.ListThreads(&threads)) {
+      case ThreadLister::Error:
         ResumeAllThreads();
         return false;
+      case ThreadLister::Incomplete:
+        added_threads = true;
+        break;
+      case ThreadLister::Ok:
+        break;
     }
     for (int tid : threads)
       if (SuspendThread(tid))
         added_threads = true;
-    if (first_iteration && !added_threads) {
-      // Detach threads and fail.
-      ResumeAllThreads();
-      return false;
-    }
-    first_iteration = false;
   } while (added_threads);
   return true;
 }
 
 // Pointer to the ThreadSuspender instance for use in signal handler.
 static ThreadSuspender *thread_suspender_instance = nullptr;
 
 // Synchronous signals that should not be blocked.
 static const int kSyncSignals[] = { SIGABRT, SIGILL, SIGFPE, SIGSEGV, SIGBUS,