[compiler-rt] Avoid truncating Symbolizer output

[paulkirth]

Summary

Repalce the fixed buffer in SymbolizerProcess with InternalScopedString,
and simply append to it when reading data.

Fixes #55460

[paulkirth]

@vitalybuka is this change congruent w/ what you were suggesting in D126102?

[vitalybuka]

Thanks.
LGTM with few nits

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_internal.h

96

getBuff don't need to be virtual

96

please make getBuff -> GetBuff
make it protected and return Vector&

117

String is used to do formated append()

For this task InternalMmapVector<char> looks more convenient.

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp

515–543

we don't need another buffer
I didn't compile, but this is idea:

bool SymbolizerProcess::ReadFromSymbolizer() {
  buffer_.clear();
  bool ret = true;
  do {
    uptr just_read = 0;
    constexpr uptr max_length = 256;
    uptr size_before = buffer_.size();
    buffer_.resize(size_before + max_length);
    bool success = ReadFromFile(input_fd_, buffer_.data(),
                                max_length, &just_read);

    if (!success)
      just_read = 0;
    buffer_.resize(size_before + just_read);

    // We can't read 0 bytes, as we don't expect external symbolizer to close
    // its stdout.
    if (just_read == 0) {
      Report("WARNING: Can't read from symbolizer at fd %d\n", input_fd_);
      ret = false;
      break;
    }
  }  while (!ReachedEndOfOutput(buffer_.data(), buffer_.length()));
  buffer_.append('\0');
  return ret;
}

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

237

Let's make it nicer, so buffer size and trailing zero position match. Something like:

GetBuff().resize(garbage - start);
GetBuff().push_back('\0');

[paulkirth]

Address comments.

Replace InternalScopedString w/ InternalMmapVector.
Use PageSize as the minimum size to read in, since we dont allocate on the
stack anymore.
Adopt suggested code changes when reading in data.

[paulkirth]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp

515–543

This was a good suggestion after moving to the vector.

[leonardchan]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp

524

nit: I think you could just use and assign to ret here instead of having success

compiler-rt/test/sanitizer_common/TestCases/symbolize_stack.cpp

29

This might be a little extra work, but could you rework the checks such that we somewhat find all 10 frames in the recursive call to this function. IIRC, this test could unexpectedly pass if it just happen to have one symbolized frame that have 5 vectors in that frame. Since the behavior we expect with this fix is that we should have the correct symbols for each frame, I think a more verbose test would be 10 checks (one for each frame) and they have the appropriate number of vectors in them. They don't have to be the fully expanded symbol name, but just enough that we capture all the vectors, so they would look something like:

CHECK: {{#[0-9]+.*vector<.*vector.*<vector.* ... >.*>.*>}}  // Or however number of `vector`s are expected

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp

518

GetPageSizeCached?
but better simple constant like 256, to avoid relying on Vector details
Vector will start with PageSize

You can do the following to use entire buffer:
buffer_.resize(size_before + max_length);
buffer_.resize(buffer_.capacity());

525

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

244

why do you need reinterpret cast?
both are "const char*"
also resize needs number of elements, not address diff, so if char was 2 bytes (garbage - buff.data()) still stands, when reinterpret is incorrect.

[paulkirth]

Address comments

• use the original buffer size(1024) as a fixed size increment when increasing buffer size.
• make full capacity availible when reading in data.
• replace success with ret.
• refactor pointer arithmetic.

[paulkirth]

Update test condition per discussion with @leonardchan

• match the beggining of the line, some number of vectors, and the file location

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_internal.h

  protected:
   ~SymbolizerProcess() {}
 
   /// The maximum number of arguments required to invoke a tool process.
   static const unsigned kArgVMax = 16;
 
   // Customizable by subclasses.
   virtual bool StartSymbolizerSubprocess();
-  virtual bool ReadFromSymbolizer(char *buffer, uptr max_length);
+  virtual bool ReadFromSymbolizer();
   // Return the environment to run the symbolizer in.
   virtual char **GetEnvP() { return GetEnviron(); }
+  InternalMmapVector<char> &GetBuff() { return buffer_; }
 
  private:
   virtual bool ReachedEndOfOutput(const char *buffer, uptr length) const {
     UNIMPLEMENTED();
   }
 
   /// Fill in an argv array to invoke the child process.
   virtual void GetArgV(const char *path_to_binary,
                        const char *(&argv)[kArgVMax]) const {
     UNIMPLEMENTED();
   }
 
   bool Restart();
   const char *SendCommandImpl(const char *command);
   bool WriteToSymbolizer(const char *buffer, uptr length);
 
   const char *path_;
   fd_t input_fd_;
   fd_t output_fd_;
 
-  static const uptr kBufferSize = 16 * 1024;
-  char buffer_[kBufferSize];
+  InternalMmapVector<char> buffer_;
 
   static const uptr kMaxTimesRestarted = 5;
   static const int kSymbolizerStartupTimeMillis = 10;
   uptr times_restarted_;
   bool failed_to_start_;
   bool reported_invalid_path_;
   bool use_posix_spawn_;
 };

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp

const char *SymbolizerProcess::SendCommand(const char *command) {
   return nullptr;
 }
 
 const char *SymbolizerProcess::SendCommandImpl(const char *command) {
   if (input_fd_ == kInvalidFd || output_fd_ == kInvalidFd)
       return nullptr;
   if (!WriteToSymbolizer(command, internal_strlen(command)))
       return nullptr;
-  if (!ReadFromSymbolizer(buffer_, kBufferSize))
-      return nullptr;
-  return buffer_;
+  if (!ReadFromSymbolizer())
+  return buffer_.data();
 }
 
 bool SymbolizerProcess::Restart() {
   if (input_fd_ != kInvalidFd)
     CloseFile(input_fd_);
   if (output_fd_ != kInvalidFd)
     CloseFile(output_fd_);
   return StartSymbolizerSubprocess();
 }
 
-bool SymbolizerProcess::ReadFromSymbolizer(char *buffer, uptr max_length) {
-  if (max_length == 0)
-    return true;
-  uptr read_len = 0;
-  while (true) {
+bool SymbolizerProcess::ReadFromSymbolizer() {
+  buffer_.clear();
+  constexpr uptr max_length = 1024;
+  bool ret = true;
+  do {
     uptr just_read = 0;
-    bool success = ReadFromFile(input_fd_, buffer + read_len,
-                                max_length - read_len - 1, &just_read);
+    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);
+
+    if (!ret)
+      just_read = 0;
+
+    buffer_.resize(size_before + just_read);
+
     // We can't read 0 bytes, as we don't expect external symbolizer to close
     // its stdout.
-    if (!success || just_read == 0) {
+    if (just_read == 0) {
       Report("WARNING: Can't read from symbolizer at fd %d\n", input_fd_);
-      return false;
-    }
-    read_len += just_read;
-    if (ReachedEndOfOutput(buffer, read_len))
-      break;
-    if (read_len + 1 == max_length) {
-      Report("WARNING: Symbolizer buffer too small\n");
-      read_len = 0;
+      ret = false;
       break;
     }
-  }
-  buffer[read_len] = '\0';
-  return true;
+  } while (!ReachedEndOfOutput(buffer_.data(), buffer_.size()));
+  buffer_.push_back('\0');
+  return ret;
 }
 
 bool SymbolizerProcess::WriteToSymbolizer(const char *buffer, uptr length) {
   if (length == 0)
     return true;
   uptr write_len = 0;
   bool success = WriteToFile(output_fd_, buffer, length, &write_len);
   if (!success || write_len != length) {
     Report("WARNING: Can't write to symbolizer at fd %d\n", output_fd_);
     return false;
   }
   return true;
 }
 
 #endif  // !SANITIZER_SYMBOLIZER_MARKUP
 
 }  // namespace __sanitizer

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

void GetArgV(const char *path_to_binary,
     argv[i++] = "-fe";
     argv[i++] = module_name_;
     argv[i++] = nullptr;
     CHECK_LE(i, kArgVMax);
   }
 
   bool ReachedEndOfOutput(const char *buffer, uptr length) const override;
 
-  bool ReadFromSymbolizer(char *buffer, uptr max_length) override {
-    if (!SymbolizerProcess::ReadFromSymbolizer(buffer, max_length))
+  bool ReadFromSymbolizer() override {
+    if (!SymbolizerProcess::ReadFromSymbolizer())
       return false;
-    // The returned buffer is empty when output is valid, but exceeds
-    // max_length.
-    if (*buffer == '\0')
-      return true;
+    auto &buff = GetBuff();
     // We should cut out output_terminator_ at the end of given buffer,
     // appended by addr2line to mark the end of its meaningful output.
     // We cannot scan buffer from it's beginning, because it is legal for it
     // to start with output_terminator_ in case given offset is invalid. So,
     // scanning from second character.
-    char *garbage = internal_strstr(buffer + 1, output_terminator_);
+    char *garbage = internal_strstr(buff.data() + 1, output_terminator_);
     // This should never be NULL since buffer must end up with
     // output_terminator_.
     CHECK(garbage);
+
     // Trim the buffer.
-    garbage[0] = '\0';
+    uintptr_t new_size = garbage - buff.data();
+    GetBuff().resize(new_size);
+    GetBuff().push_back('\0');
     return true;
   }
 
   const char *module_name_;  // Owned, leaked.
   static const char output_terminator_[];
 };
 
 const char Addr2LineProcess::output_terminator_[] = "??\n??:0\n";

compiler-rt/test/sanitizer_common/TestCases/symbolize_stack.cpp

 // RUN: %clangxx -O0 %s -o %t && %run %t 2>&1 | FileCheck %s
 
 // Test that symbolizer does not crash on frame with large function name.
 
 // On Darwin LSan reports a false positive
 // XFAIL: darwin && lsan
 
-// FIXME: https://github.com/llvm/llvm-project/issues/55460
-// On Linux its possible for symbolizer output to be truncated and to match the
-// check below. Remove when the underlying problem has been addressed.
-// UNSUPPORTED: linux
-
 #include <sanitizer/common_interface_defs.h>
 #include <vector>
 
 template <int N> struct A {
   template <class T> void RecursiveTemplateFunction(const T &t);
 };
 
 template <int N>
 template <class T>
 __attribute__((noinline)) void A<N>::RecursiveTemplateFunction(const T &) {
   std::vector<T> t;
   return A<N - 1>().RecursiveTemplateFunction(t);
 }
 
 template <>
 template <class T>
 __attribute__((noinline)) void A<0>::RecursiveTemplateFunction(const T &) {
   __sanitizer_print_stack_trace();
 }
 
 int main() {
-  // CHECK: {{vector<.*vector<.*vector<.*vector<.*vector<}}
+  // CHECK: {{#[0-9]+.*A<0>.*vector<.*vector<.*vector<.*vector<.*vector<.*vector<.*vector<.*vector<.*vector<.*vector<.*vector<.*vector.*symbolize_stack.cpp:25}}
   A<10>().RecursiveTemplateFunction(0);
 }