[compiler-rt] [sanitizer_common] fix shadowed ret hiding symbolizer read failures. (PR #209971)

[via llvm-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

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

Author: David CARLIER (devnexen)

<details>
<summary>Changes</summary>

The inner `bool ret` from ReadFromFile shadowed the outer one that is returned, so a dead/closed symbolizer (0-byte read or read error) was reported as success: SendCommand never restarted the crashed process, and the addr2line path aborted on a CHECK over an empty buffer. Regression from acfeb1a6c244. Add a regression test that feeds an EOF fd through ReadFromSymbolizer and expects failure.

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


3 Files Affected:

- (modified) compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_internal.h (+3) 
- (modified) compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp (+2-2) 
- (modified) compiler-rt/lib/sanitizer_common/tests/sanitizer_symbolizer_test.cpp (+27) 


``````````diff
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_internal.h b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_internal.h
index 6442a2980bf2f..c84b0ad1ec243 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_internal.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_internal.h
@@ -96,6 +96,9 @@ class SymbolizerProcess {
   InternalMmapVector<char> &GetBuff() { return buffer_; }
 
  private:
+  // Grants the unit test access to input_fd_ so it can exercise the read loop.
+  friend class TestSymbolizerProcess;
+
   virtual bool ReachedEndOfOutput(const char *buffer, uptr length) const {
     UNIMPLEMENTED();
   }
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp
index cc31d3d8056f9..a6264d8229e49 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp
@@ -555,8 +555,8 @@ bool SymbolizerProcess::ReadFromSymbolizer() {
     uptr size_before = buffer_.size();
     buffer_.resize(size_before + max_length);
     buffer_.resize(buffer_.capacity());
-    bool ret = ReadFromFile(input_fd_, &buffer_[size_before],
-                            buffer_.size() - size_before, &just_read);
+    ret = ReadFromFile(input_fd_, &buffer_[size_before],
+                       buffer_.size() - size_before, &just_read);
 
     if (!ret)
       just_read = 0;
diff --git a/compiler-rt/lib/sanitizer_common/tests/sanitizer_symbolizer_test.cpp b/compiler-rt/lib/sanitizer_common/tests/sanitizer_symbolizer_test.cpp
index b8267e4007997..e03af4c71d222 100644
--- a/compiler-rt/lib/sanitizer_common/tests/sanitizer_symbolizer_test.cpp
+++ b/compiler-rt/lib/sanitizer_common/tests/sanitizer_symbolizer_test.cpp
@@ -68,4 +68,31 @@ TEST(Symbolizer, DemangleSwiftAndCXX) {
 }
 #endif
 
+#if SANITIZER_POSIX
+// Must live directly in __sanitizer (not an anonymous namespace) to match the
+// `friend class TestSymbolizerProcess` declaration in SymbolizerProcess.
+class TestSymbolizerProcess final : public SymbolizerProcess {
+ public:
+  TestSymbolizerProcess() : SymbolizerProcess("/invalid/symbolizer/path") {}
+  bool ReadFromFd(fd_t fd) {
+    input_fd_ = fd;
+    return ReadFromSymbolizer();
+  }
+
+ private:
+  bool ReachedEndOfOutput(const char *, uptr) const override { return true; }
+};
+
+// A symbolizer that closes its stdout yields a 0-byte (EOF) read. That must be
+// reported as a failure, not silently accepted as success. Regression test for
+// the shadowed `ret` in SymbolizerProcess::ReadFromSymbolizer.
+TEST(Symbolizer, ReadFromSymbolizerReportsEofAsFailure) {
+  fd_t fd = OpenFile("/dev/null", RdOnly);
+  ASSERT_NE(fd, kInvalidFd);
+  TestSymbolizerProcess symbolizer;
+  EXPECT_FALSE(symbolizer.ReadFromFd(fd));
+  CloseFile(fd);
+}
+#endif  // SANITIZER_POSIX
+
 }  // namespace __sanitizer

``````````

</details>


https://github.com/llvm/llvm-project/pull/209971