[sanitizer] Cleanup ReadFileToVector and ReadFileToBuffer

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

Reviewers: eugenis

Subscribers: kubamracek, mgorny, llvm-commits

Differential Revision: https://reviews.llvm.org/D46618

git-svn-id: https://llvm.org/svn/llvm-project/compiler-rt/trunk@334130 91177308-0d34-0410-b5e6-96231b3b80d8

5 files changed, 109 insertions, 45 deletions

diff --git a/lib/sanitizer_common/sanitizer_common.h b/lib/sanitizer_common/sanitizer_common.h
index 4246d2e70..5408c6b50 100644
--- a/lib/sanitizer_common/sanitizer_common.h
+++ b/lib/sanitizer_common/sanitizer_common.h
@@ -639,18 +639,22 @@ enum ModuleArch {
 
 // 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.
diff --git a/lib/sanitizer_common/sanitizer_file.cc b/lib/sanitizer_common/sanitizer_file.cc
index 665802967..278d75c52 100644
--- a/lib/sanitizer_common/sanitizer_file.cc
+++ b/lib/sanitizer_common/sanitizer_file.cc
@@ -95,34 +95,40 @@ void ReportFile::SetReportPath(const char *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.
@@ -131,40 +137,34 @@ bool ReadFileToBuffer(const char *file_name, char **buff, uptr *buff_size,
   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)
+  if (!max_len)
+    return true;
+  uptr PageSize = GetPageSizeCached();
+  fd_t fd = OpenFile(file_name, RdOnly, errno_p);
+  if (fd == kInvalidFd)
+    return false;
+  uptr read_len = 0;
+  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);
+    uptr just_read;
+    if (!ReadFromFile(fd, buff->data() + read_len, buff->size() - read_len,
+                      &just_read, errno_p)) {
+      CloseFile(fd);
       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);
+    read_len += just_read;
+    if (!just_read)
       break;
-    }
   }
+  CloseFile(fd);
+  buff->resize(read_len);
   return true;
 }
 
diff --git a/lib/sanitizer_common/sanitizer_linux.cc b/lib/sanitizer_common/sanitizer_linux.cc
index a5b9dd008..63866af63 100644
--- a/lib/sanitizer_common/sanitizer_linux.cc
+++ b/lib/sanitizer_common/sanitizer_linux.cc
@@ -975,7 +975,7 @@ bool ThreadLister::IsAlive(int tid) {
   // 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:";
diff --git a/lib/sanitizer_common/tests/CMakeLists.txt b/lib/sanitizer_common/tests/CMakeLists.txt
index 9f2cb39fb..4b8a0f2c8 100644
--- a/lib/sanitizer_common/tests/CMakeLists.txt
+++ b/lib/sanitizer_common/tests/CMakeLists.txt
@@ -54,7 +54,8 @@ set(SANITIZER_TEST_CFLAGS_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.
diff --git a/lib/sanitizer_common/tests/sanitizer_libc_test.cc b/lib/sanitizer_common/tests/sanitizer_libc_test.cc
index a73c65a51..1a9add857 100644
--- a/lib/sanitizer_common/tests/sanitizer_libc_test.cc
+++ b/lib/sanitizer_common/tests/sanitizer_libc_test.cc
@@ -9,6 +9,7 @@
 // Tests for sanitizer_libc.h.
 //===----------------------------------------------------------------------===//
 #include <algorithm>
+#include <fstream>
 
 #include "sanitizer_common/sanitizer_common.h"
 #include "sanitizer_common/sanitizer_file.h"
@@ -82,8 +83,8 @@ static void temp_file_name(char *buf, size_t bufsize, const char *prefix) {
   // 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
 }
 
@@ -151,6 +152,64 @@ TEST(SanitizerCommon, FileOps) {
 #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;