From 4ef9117b49edd323ed07f0c4a3480fb6c53fbc21 Mon Sep 17 00:00:00 2001 From: Vitaly Buka Date: Wed, 6 Jun 2018 20:53:43 +0000 Subject: [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 llvm-svn: 334130 --- .../lib/sanitizer_common/sanitizer_common.h | 16 +++-- compiler-rt/lib/sanitizer_common/sanitizer_file.cc | 70 +++++++++++----------- .../lib/sanitizer_common/sanitizer_linux.cc | 2 +- .../lib/sanitizer_common/tests/CMakeLists.txt | 3 +- .../sanitizer_common/tests/sanitizer_libc_test.cc | 63 ++++++++++++++++++- 5 files changed, 109 insertions(+), 45 deletions(-) diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_common.h b/compiler-rt/lib/sanitizer_common/sanitizer_common.h index 4246d2e7002..5408c6b5055 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_common.h +++ b/compiler-rt/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 *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 *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/compiler-rt/lib/sanitizer_common/sanitizer_file.cc b/compiler-rt/lib/sanitizer_common/sanitizer_file.cc index 66580296788..278d75c520a 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_file.cc +++ b/compiler-rt/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 *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/compiler-rt/lib/sanitizer_common/sanitizer_linux.cc b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cc index a5b9dd00898..63866af6338 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cc +++ b/compiler-rt/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/compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt b/compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt index 9f2cb39fb53..4b8a0f2c8be 100644 --- a/compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt +++ b/compiler-rt/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/compiler-rt/lib/sanitizer_common/tests/sanitizer_libc_test.cc b/compiler-rt/lib/sanitizer_common/tests/sanitizer_libc_test.cc index a73c65a510c..1a9add857ba 100644 --- a/compiler-rt/lib/sanitizer_common/tests/sanitizer_libc_test.cc +++ b/compiler-rt/lib/sanitizer_common/tests/sanitizer_libc_test.cc @@ -9,6 +9,7 @@ // Tests for sanitizer_libc.h. //===----------------------------------------------------------------------===// #include +#include #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 { + 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 data_; +}; + +TEST_P(SanitizerCommonFileTest, ReadFileToBuffer) { + char *buff; + uptr size; + uptr len; + EXPECT_TRUE(ReadFileToBuffer(file_name_, &buff, &len, &size)); + EXPECT_EQ(data_, std::vector(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(buff, buff + size)); + UnmapOrDie(buff, len); +} + +TEST_P(SanitizerCommonFileTest, ReadFileToVector) { + InternalMmapVector buff; + EXPECT_TRUE(ReadFileToVector(file_name_, &buff)); + EXPECT_EQ(data_, std::vector(buff.begin(), buff.end())); +} + +TEST_P(SanitizerCommonFileTest, ReadFileToVectorHalf) { + InternalMmapVector buff; + data_.resize(data_.size() / 2); + EXPECT_TRUE(ReadFileToVector(file_name_, &buff, data_.size())); + EXPECT_EQ(data_, std::vector(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; -- cgit v1.2.1