[NFC][sanitizer] Reopen '/proc/%d/task' instead of seek

[vitalybuka]

NFC because I am not aware of any particular
issue from seek, but reopen looks less error prone.

[llvmbot]

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

Author: Vitaly Buka (vitalybuka)

Changes

NFC because I am not aware of any particular
issue from seek, but reopen looks less error prone.

---

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

2 Files Affected:

• (modified) compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp (+13-19)
• (modified) compiler-rt/lib/sanitizer_common/sanitizer_linux.h (+2-3)

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
index d421d117e67274..671546cf3c4fa3 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
@@ -1025,21 +1025,21 @@ bool internal_sigismember(__sanitizer_sigset_t *set, int signum) {
 
 #  if !SANITIZER_NETBSD
 // ThreadLister implementation.
-ThreadLister::ThreadLister(pid_t pid) : pid_(pid), buffer_(4096) {
-  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);
-  }
+ThreadLister::ThreadLister(pid_t pid) : buffer_(4096) {
+  task_path_.AppendF("/proc/%d/task", pid);
+  status_path_.AppendF("%s/status", task_path_.data());
 }
 
 ThreadLister::Result ThreadLister::ListThreads(
     InternalMmapVector<tid_t> *threads) {
-  if (internal_iserror(descriptor_))
+  int descriptor = internal_open(task_path_.data(), O_RDONLY | O_DIRECTORY);
+  if (internal_iserror(descriptor)) {
+    Report("Can't open %s for reading.\n", task_path_.data());
     return Error;
-  internal_lseek(descriptor_, 0, SEEK_SET);
+  }
+  auto acts_cleanup = at_scope_exit([&] {
+    internal_close(descriptor);
+  });
   threads->clear();
 
   Result result = Ok;
@@ -1048,11 +1048,11 @@ ThreadLister::Result ThreadLister::ListThreads(
     buffer_.resize(buffer_.capacity());
     CHECK_GE(buffer_.size(), 4096);
     uptr read = internal_getdents(
-        descriptor_, (struct linux_dirent *)buffer_.data(), buffer_.size());
+        descriptor, (struct linux_dirent *)buffer_.data(), buffer_.size());
     if (!read)
       return result;
     if (internal_iserror(read)) {
-      Report("Can't read directory entries from /proc/%d/task.\n", pid_);
+      Report("Can't read directory entries from %s.\n", task_path_.data());
       return Error;
     }
 
@@ -1093,9 +1093,7 @@ ThreadLister::Result ThreadLister::ListThreads(
 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())
+  if (!ReadFileToVector(status_path_.data(), &buffer_) || buffer_.empty())
     return false;
   buffer_.push_back(0);
   static const char kPrefix[] = "\nPPid:";
@@ -1106,10 +1104,6 @@ bool ThreadLister::IsAlive(int tid) {
   return (int)internal_atoll(field) != 0;
 }
 
-ThreadLister::~ThreadLister() {
-  if (!internal_iserror(descriptor_))
-    internal_close(descriptor_);
-}
 #  endif
 
 #  if SANITIZER_WORDSIZE == 32
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux.h b/compiler-rt/lib/sanitizer_common/sanitizer_linux.h
index c30f0326793d5a..96c617822b5b27 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_linux.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux.h
@@ -97,7 +97,6 @@ uptr internal_clone(int (*fn)(void *), void *child_stack, int flags, void *arg);
 class ThreadLister {
  public:
   explicit ThreadLister(pid_t pid);
-  ~ThreadLister();
   enum Result {
     Error,
     Incomplete,
@@ -108,8 +107,8 @@ class ThreadLister {
  private:
   bool IsAlive(int tid);
 
-  pid_t pid_;
-  int descriptor_ = -1;
+  InternalScopedString task_path_;
+  InternalScopedString status_path_;
   InternalMmapVector<char> buffer_;
 };
 

[fmayer]

LGTM, what is the motivation for this?

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 07892aaf04032e7a18368bc8320f93f7d46ab20f 877288bf8d9ddd83bf88377f26fb87ac74a88132 --extensions cpp,h -- compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp compiler-rt/lib/sanitizer_common/sanitizer_linux.h

View the diff from clang-format here.

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
index 671546cf3c..70fd9405e5 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
@@ -1037,9 +1037,7 @@ ThreadLister::Result ThreadLister::ListThreads(
     Report("Can't open %s for reading.\n", task_path_.data());
     return Error;
   }
-  auto acts_cleanup = at_scope_exit([&] {
-    internal_close(descriptor);
-  });
+  auto acts_cleanup = at_scope_exit([&] { internal_close(descriptor); });
   threads->clear();
 
   Result result = Ok;

[vitalybuka]

LGTM, what is the motivation for this?

Browsing code looking for cause of false leak reports.

[nico]

LGTM, what is the motivation for this?

Browsing code looking for cause of false leak reports.

This broke some of our tests: https://crbug.com/375489584 Could you take a look?