[sanitizer] Cleanup ReadFileToVector and ReadFileToBuffer

[vitalybuka]

Summary

Added unit-test.
Fixed behavior of max_len argument.
Call read syscall with all available buffer, not just a page.

[vitalybuka]

rebase

[morehouse]

compiler-rt/lib/sanitizer_common/sanitizer_common.h

648

Why is it implemented this way rather than copying over the bytes that have already been read?

compiler-rt/lib/sanitizer_common/sanitizer_file.cc

97–98

This function looks much better than before, but since we're already modifying this maybe it would make sense to copy data over to the new mmapped region rather than rereading it.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_common.h

648

Updated comment.

compiler-rt/lib/sanitizer_common/sanitizer_file.cc

97–98

we can't as we want to discard results after each short read for proc maps

[vitalybuka]

comments

[morehouse]

Just found https://github.com/google/sanitizers/issues/435 and https://chromiumcodereview.appspot.com/18661009.

Maybe it would make sense to write a separate function for reading procmaps.

[vitalybuka]

In D46618#1124051, @morehouse wrote:

Just found https://github.com/google/sanitizers/issues/435 and https://chromiumcodereview.appspot.com/18661009.

Maybe it would make sense to write a separate function for reading procmaps.

That's the plan.
I'll switch all non-procmaps callers to ReadFileToVector and then rename/hide ReadFileToBuffer to something procmaps specific.

compiler-rt/lib/sanitizer_common/sanitizer_common.h

enum ModuleArch {
   kModuleArchARMV7,
   kModuleArchARMV7S,
   kModuleArchARMV7K,
   kModuleArchARM64
 };
 
 // Opens the file 'file_name" and reads up to 'max_len' bytes.
 // The resulting buffer is mmaped and stored in '*buff'.
+// Returns true if file was successfully opened and read.
+bool ReadFileToVector(const char *file_name,
+                      InternalMmapVectorNoCtor<char> *buff,
+                      uptr max_len = 1 << 26, error_t *errno_p = nullptr);
+
+// Opens the file 'file_name" and reads up to 'max_len' bytes.
+// This function is less I/O efficient than ReadFileToVector as it may reread
+// file multiple times to avoid mmap during read attempts. It's used to read
+// procmap, so short reads with mmap in between can produce inconsistent result.
+// The resulting buffer is mmaped and stored in '*buff'.
 // The size of the mmaped region is stored in '*buff_size'.
 // The total number of read bytes is stored in '*read_len'.
 // Returns true if file was successfully opened and read.
 bool ReadFileToBuffer(const char *file_name, char **buff, uptr *buff_size,
                       uptr *read_len, uptr max_len = 1 << 26,
                       error_t *errno_p = nullptr);
-// Opens the file 'file_name" and reads up to 'max_len' bytes.
-// The resulting buffer is mmaped and stored in '*buff'.
-// Returns true if file was successfully opened and read.
-bool ReadFileToBuffer(const char *file_name,
-                      InternalMmapVectorNoCtor<char> *buff,
-                      uptr max_len = 1 << 26, error_t *errno_p = nullptr);
 
 // When adding a new architecture, don't forget to also update
 // script/asan_symbolize.py and sanitizer_symbolizer_libcdep.cc.
 inline const char *ModuleArchToString(ModuleArch arch) {
   switch (arch) {
     case kModuleArchUnknown:
       return "";
     case kModuleArchI386:

compiler-rt/lib/sanitizer_common/sanitizer_file.cc

void ReportFile::SetReportPath(const char *path) {
   } else if (internal_strcmp(path, "stderr") == 0) {
     fd = kStderrFd;
   } else {
     internal_snprintf(path_prefix, kMaxPathLength, "%s", path);
   }
 }
 
 bool ReadFileToBuffer(const char *file_name, char **buff, uptr *buff_size,
                       uptr *read_len, uptr max_len, error_t *errno_p) {
-  uptr PageSize = GetPageSizeCached();
-  uptr kMinFileLen = PageSize;
   *buff = nullptr;
   *buff_size = 0;
   *read_len = 0;
+  if (!max_len)
+    return true;
+  uptr PageSize = GetPageSizeCached();
+  uptr kMinFileLen = Min(PageSize, max_len);
+
   // The files we usually open are not seekable, so try different buffer sizes.
-  for (uptr size = kMinFileLen; size <= max_len; size *= 2) {
+  for (uptr size = kMinFileLen;; size = Min(size * 2, max_len)) {
     UnmapOrDie(*buff, *buff_size);
     *buff = (char*)MmapOrDie(size, __func__);
     *buff_size = size;
     fd_t fd = OpenFile(file_name, RdOnly, errno_p);
-    if (fd == kInvalidFd)
+    if (fd == kInvalidFd) {
+      UnmapOrDie(*buff, *buff_size);
       return false;
+    }
     *read_len = 0;
     // Read up to one page at a time.
     bool reached_eof = false;
-    while (*read_len + PageSize <= size) {
+    while (*read_len < size) {
       uptr just_read;
-      if (!ReadFromFile(fd, *buff + *read_len, PageSize, &just_read, errno_p)) {
+      if (!ReadFromFile(fd, *buff + *read_len, size - *read_len, &just_read,
+                        errno_p)) {
         UnmapOrDie(*buff, *buff_size);
         CloseFile(fd);
         return false;
       }
-      if (just_read == 0) {
+      *read_len += just_read;
+      if (just_read == 0 || *read_len == max_len) {
         reached_eof = true;
         break;
       }
-      *read_len += just_read;
     }
     CloseFile(fd);
     if (reached_eof)  // We've read the whole file.
       break;
   }
   return true;
 }
 
-bool ReadFileToBuffer(const char *file_name,
+bool ReadFileToVector(const char *file_name,
                       InternalMmapVectorNoCtor<char> *buff, uptr max_len,
                       error_t *errno_p) {
-  uptr PageSize = GetPageSizeCached();
   buff->clear();
-  // The files we usually open are not seekable, so try different buffer sizes.
-  for (uptr size = Max(PageSize, buff->capacity()); size <= max_len;
-       size *= 2) {
-    buff->resize(size);
-    fd_t fd = OpenFile(file_name, RdOnly, errno_p);
-    if (fd == kInvalidFd)
-      return false;
-    uptr read_len = 0;
-    // Read up to one page at a time.
-    bool reached_eof = false;
-    while (read_len + PageSize <= buff->size()) {
-      uptr just_read;
-      if (!ReadFromFile(fd, buff->data() + read_len, PageSize, &just_read,
-                        errno_p)) {
-        CloseFile(fd);
-        return false;
-      }
-      if (!just_read) {
-        reached_eof = true;
-        break;
-      }
-      read_len += just_read;
-    }
-    CloseFile(fd);
-    if (reached_eof) {  // We've read the whole file.
-      buff->resize(read_len);
-      break;
-    }
-  }
+  if (!max_len)
+    return true;
+  uptr PageSize = GetPageSizeCached();
+  while (read_len < max_len) {
+    if (read_len >= buff->size())
+      buff->resize(Min(Max(PageSize, read_len * 2), max_len));
+    CHECK_LT(read_len, buff->size());
+    CHECK_LE(buff->size(), max_len);
+    if (!ReadFromFile(fd, buff->data() + read_len, buff->size() - read_len,
+                      &just_read, errno_p)) {
+    if (!just_read)
+      break;
   return true;
 }
 
 static const char kPathSeparator = SANITIZER_WINDOWS ? ';' : ':';
 
 char *FindPathToBinary(const char *name) {
   if (FileExists(name)) {
     return internal_strdup(name);

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

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 (!ReadFileToBuffer(path, &buffer_) || buffer_.empty())
+  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;

compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt

set(SANITIZER_TEST_CFLAGS_COMMON
   ${COMPILER_RT_UNITTEST_CFLAGS}
   ${COMPILER_RT_GTEST_CFLAGS}
   -I${COMPILER_RT_SOURCE_DIR}/include
   -I${COMPILER_RT_SOURCE_DIR}/lib
   -I${COMPILER_RT_SOURCE_DIR}/lib/sanitizer_common
   -fno-rtti
   -O2
   -Werror=sign-compare
-  -Wno-non-virtual-dtor)
+  -Wno-non-virtual-dtor
+  -Wno-gnu-zero-variadic-macro-arguments)
 
 if(MSVC)
   # Disable exceptions on Windows until they work reliably.
   list(APPEND SANITIZER_TEST_CFLAGS_COMMON -fno-exceptions -DGTEST_HAS_SEH=0)
 endif()
 
 # -gline-tables-only must be enough for these tests, so use it if possible.
 if(COMPILER_RT_TEST_COMPILER_ID MATCHES "Clang")

compiler-rt/lib/sanitizer_common/tests/sanitizer_libc_test.cc

 //===-- sanitizer_libc_test.cc --------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 // Tests for sanitizer_libc.h.
 //===----------------------------------------------------------------------===//
 #include <algorithm>
+#include <fstream>
 
 #include "sanitizer_common/sanitizer_common.h"
 #include "sanitizer_common/sanitizer_file.h"
 #include "sanitizer_common/sanitizer_libc.h"
 #include "sanitizer_common/sanitizer_platform.h"
 #include "gtest/gtest.h"
 
 #if SANITIZER_WINDOWS
#else
   const char *tmpdir = "/tmp";
 #if SANITIZER_ANDROID
   // I don't know a way to query temp directory location on Android without
   // going through Java interfaces. The code below is not ideal, but should
   // work. May require "adb root", but it is needed for almost any use of ASan
   // on Android already.
   tmpdir = GetEnv("EXTERNAL_STORAGE");
 #endif
-  u32 uid = GetUid();
-  internal_snprintf(buf, bufsize, "%s/%s%d", tmpdir, prefix, uid);
+  internal_snprintf(buf, bufsize, "%s/%sXXXXXX", tmpdir, prefix);
+  ASSERT_TRUE(mktemp(buf));
 #endif
 }
 
 TEST(SanitizerCommon, FileOps) {
   const char *str1 = "qwerty";
   uptr len1 = internal_strlen(str1);
   const char *str2 = "zxcv";
   uptr len2 = internal_strlen(str2);
#if SANITIZER_WINDOWS
   // No sanitizer needs to delete a file on Windows yet. If we ever do, we can
   // add a portable wrapper and test it from here.
   ::DeleteFileA(&tmpfile[0]);
 #else
   internal_unlink(tmpfile);
 #endif
 }
 
+class SanitizerCommonFileTest : public ::testing::TestWithParam<uptr> {
+  void SetUp() override {
+    data_.resize(GetParam());
+    std::generate(data_.begin(), data_.end(), [] {
+      return rand() % 256;  // NOLINT
+    });
+
+    temp_file_name(file_name_, sizeof(file_name_),
+                   "sanitizer_common.ReadFile.tmp.");
+
+    std::ofstream f(file_name_, std::ios::out | std::ios::binary);
+    if (!data_.empty())
+      f.write(data_.data(), data_.size());
+  }
+
+  void TearDown() override { internal_unlink(file_name_); }
+
+ protected:
+  char file_name_[256];
+  std::vector<char> data_;
+};
+
+TEST_P(SanitizerCommonFileTest, ReadFileToBuffer) {
+  char *buff;
+  uptr size;
+  uptr len;
+  EXPECT_TRUE(ReadFileToBuffer(file_name_, &buff, &len, &size));
+  EXPECT_EQ(data_, std::vector<char>(buff, buff + size));
+  UnmapOrDie(buff, len);
+}
+
+TEST_P(SanitizerCommonFileTest, ReadFileToBufferHalf) {
+  char *buff;
+  uptr size;
+  uptr len;
+  data_.resize(data_.size() / 2);
+  EXPECT_TRUE(ReadFileToBuffer(file_name_, &buff, &len, &size, data_.size()));
+  EXPECT_EQ(data_, std::vector<char>(buff, buff + size));
+  UnmapOrDie(buff, len);
+}
+
+TEST_P(SanitizerCommonFileTest, ReadFileToVector) {
+  InternalMmapVector<char> buff;
+  EXPECT_TRUE(ReadFileToVector(file_name_, &buff));
+  EXPECT_EQ(data_, std::vector<char>(buff.begin(), buff.end()));
+}
+
+TEST_P(SanitizerCommonFileTest, ReadFileToVectorHalf) {
+  InternalMmapVector<char> buff;
+  data_.resize(data_.size() / 2);
+  EXPECT_TRUE(ReadFileToVector(file_name_, &buff, data_.size()));
+  EXPECT_EQ(data_, std::vector<char>(buff.begin(), buff.end()));
+}
+
+INSTANTIATE_TEST_CASE_P(FileSizes, SanitizerCommonFileTest,
+                        ::testing::Values(0, 1, 7, 13, 32, 4096, 4097, 1048575,
+                                          1048576, 1048577));
+
 static const size_t kStrlcpyBufSize = 8;
 void test_internal_strlcpy(char *dbuf, const char *sbuf) {
   uptr retval = 0;
   retval = internal_strlcpy(dbuf, sbuf, kStrlcpyBufSize);
   EXPECT_EQ(internal_strncmp(dbuf, sbuf, kStrlcpyBufSize - 1), 0);
   EXPECT_EQ(internal_strlen(dbuf),
             std::min(internal_strlen(sbuf), (uptr)(kStrlcpyBufSize - 1)));
   EXPECT_EQ(retval, internal_strlen(sbuf));