From 35964ca66ac8683a1f63a1633f97ba4da4a65d94 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Tue, 14 Nov 2023 19:33:55 -0800 Subject: Change MemoryBuffer to use a vector. In addition, add an offset value so that this can be used for the compressed section data. The modified MemoryBuffer now has a max size to prevent corrupted elf data from causing huge allocations in the vector. This class is only used for global data and a compressed .debug_frame in the library code. The limit of 10MB is way over what any valid existing globals data section is expected to be. A 50MB shared library only contains a .debug_frame that is < 100KB in size. Therefore, 10MB should be able to handle any valid large shared library with a valid large .debug_frame. MemoryBuffer is used for some tests too, but those are all using relatively small values. Bug: 309857311 Test: All unit tests pass. Change-Id: I184efea87d7c1f74202071e4943c2a762435ac11 --- libunwindstack/DexFile.cpp | 8 ++-- libunwindstack/JitDebug.cpp | 6 +-- libunwindstack/Memory.cpp | 23 ++++++++---- libunwindstack/MemoryBuffer.h | 34 ++++++++--------- libunwindstack/tests/MemoryBufferTest.cpp | 61 +++++++++++++++++++++---------- libunwindstack/tests/MemoryXzTest.cpp | 4 +- 6 files changed, 80 insertions(+), 56 deletions(-) diff --git a/libunwindstack/DexFile.cpp b/libunwindstack/DexFile.cpp index cb2a28a..0bb069f 100644 --- a/libunwindstack/DexFile.cpp +++ b/libunwindstack/DexFile.cpp @@ -108,11 +108,9 @@ std::shared_ptr DexFile::Create(uint64_t base_addr, uint64_t file_size, } // Fallback: make copy in local buffer. - std::unique_ptr copy(new MemoryBuffer); - if (!copy->Resize(file_size)) { - return nullptr; - } - if (!memory->ReadFully(base_addr, copy->GetPtr(0), file_size)) { + std::unique_ptr copy(new MemoryBuffer(file_size)); + uint8_t* dst_ptr = copy->GetPtr(0); + if (dst_ptr == nullptr || !memory->ReadFully(base_addr, dst_ptr, file_size)) { return nullptr; } std::unique_ptr dex; diff --git a/libunwindstack/JitDebug.cpp b/libunwindstack/JitDebug.cpp index 20e981e..422071d 100644 --- a/libunwindstack/JitDebug.cpp +++ b/libunwindstack/JitDebug.cpp @@ -26,9 +26,9 @@ namespace unwindstack { template <> bool GlobalDebugInterface::Load(Maps*, std::shared_ptr& memory, uint64_t addr, uint64_t size, /*out*/ std::shared_ptr& elf) { - MemoryBuffer* buffer = new MemoryBuffer; - std::shared_ptr copy(buffer); - if (!buffer->Resize(size) || !memory->ReadFully(addr, buffer->GetPtr(0), size)) { + std::shared_ptr copy(new MemoryBuffer(size)); + uint8_t* dst_ptr = copy->GetPtr(0); + if (dst_ptr == nullptr || !memory->ReadFully(addr, dst_ptr, size)) { return false; } elf.reset(new Elf(copy)); diff --git a/libunwindstack/Memory.cpp b/libunwindstack/Memory.cpp index 29fc5cd..258c54a 100644 --- a/libunwindstack/Memory.cpp +++ b/libunwindstack/Memory.cpp @@ -228,21 +228,28 @@ std::shared_ptr Memory::CreateOfflineMemory(const uint8_t* data, uint64_ } size_t MemoryBuffer::Read(uint64_t addr, void* dst, size_t size) { - if (addr >= size_) { + if (addr < offset_) { + return 0; + } + addr -= offset_; + size_t raw_size = raw_.size(); + if (addr >= raw_size) { return 0; } - size_t bytes_left = size_ - static_cast(addr); - const unsigned char* actual_base = static_cast(raw_) + addr; + size_t bytes_left = raw_size - static_cast(addr); size_t actual_len = std::min(bytes_left, size); - - memcpy(dst, actual_base, actual_len); + memcpy(dst, &raw_[addr], actual_len); return actual_len; } -uint8_t* MemoryBuffer::GetPtr(size_t offset) { - if (offset < size_) { - return &raw_[offset]; +uint8_t* MemoryBuffer::GetPtr(size_t addr) { + if (addr < offset_) { + return nullptr; + } + addr -= offset_; + if (addr < raw_.size()) { + return &raw_[addr]; } return nullptr; } diff --git a/libunwindstack/MemoryBuffer.h b/libunwindstack/MemoryBuffer.h index a5b5743..a19b331 100644 --- a/libunwindstack/MemoryBuffer.h +++ b/libunwindstack/MemoryBuffer.h @@ -18,7 +18,6 @@ #include -#include #include #include @@ -27,31 +26,28 @@ namespace unwindstack { class MemoryBuffer : public Memory { public: - MemoryBuffer() = default; - virtual ~MemoryBuffer() { free(raw_); } + // If a size is too large, assume it's likely corrupted data, and set to zero. + MemoryBuffer(size_t size) : raw_(size > kMaxBufferSize ? 0 : size), offset_(0) {} + MemoryBuffer(size_t size, uint64_t offset) + : raw_(size > kMaxBufferSize ? 0 : size), offset_(offset) {} + virtual ~MemoryBuffer() = default; size_t Read(uint64_t addr, void* dst, size_t size) override; uint8_t* GetPtr(size_t offset) override; - bool Resize(size_t size) { - void* new_raw = realloc(raw_, size); - if (new_raw == nullptr) { - free(raw_); - raw_ = nullptr; - size_ = 0; - return false; - } - raw_ = reinterpret_cast(new_raw); - size_ = size; - return true; - } - - uint64_t Size() { return size_; } + uint64_t Size() { return raw_.size(); } private: - uint8_t* raw_ = nullptr; - size_t size_ = 0; + std::vector raw_; + uint64_t offset_; + + // This class is only used for global data and a compressed .debug_frame in + // the library code. The limit of 10MB is way over what any valid existing + // globals data section is expected to be. A 50MB shared library only contains + // a .debug_frame that is < 100KB in size. Therefore, 10MB should be able to + // handle any valid large shared library with a valid large .debug_frame. + static constexpr size_t kMaxBufferSize = 10 * 1024 * 1024; }; } // namespace unwindstack diff --git a/libunwindstack/tests/MemoryBufferTest.cpp b/libunwindstack/tests/MemoryBufferTest.cpp index 91b20dd..d375f0f 100644 --- a/libunwindstack/tests/MemoryBufferTest.cpp +++ b/libunwindstack/tests/MemoryBufferTest.cpp @@ -27,14 +27,12 @@ namespace unwindstack { class MemoryBufferTest : public ::testing::Test { protected: - void SetUp() override { - ResetLogs(); - memory_.reset(new MemoryBuffer); - } + void SetUp() override { ResetLogs(); } std::unique_ptr memory_; }; TEST_F(MemoryBufferTest, empty) { + memory_.reset(new MemoryBuffer(0)); ASSERT_EQ(0U, memory_->Size()); std::vector buffer(1024); ASSERT_FALSE(memory_->ReadFully(0, buffer.data(), 1)); @@ -43,7 +41,7 @@ TEST_F(MemoryBufferTest, empty) { } TEST_F(MemoryBufferTest, write_read) { - ASSERT_TRUE(memory_->Resize(256)); + memory_.reset(new MemoryBuffer(256)); ASSERT_EQ(256U, memory_->Size()); ASSERT_TRUE(memory_->GetPtr(0) != nullptr); ASSERT_TRUE(memory_->GetPtr(1) != nullptr); @@ -63,7 +61,8 @@ TEST_F(MemoryBufferTest, write_read) { } TEST_F(MemoryBufferTest, read_failures) { - ASSERT_TRUE(memory_->Resize(100)); + memory_.reset(new MemoryBuffer(100)); + ASSERT_EQ(100U, memory_->Size()); std::vector buffer(200); ASSERT_FALSE(memory_->ReadFully(0, buffer.data(), 101)); ASSERT_FALSE(memory_->ReadFully(100, buffer.data(), 1)); @@ -73,14 +72,14 @@ TEST_F(MemoryBufferTest, read_failures) { } TEST_F(MemoryBufferTest, read_failure_overflow) { - ASSERT_TRUE(memory_->Resize(100)); + memory_.reset(new MemoryBuffer(100)); + ASSERT_EQ(100U, memory_->Size()); std::vector buffer(200); - ASSERT_FALSE(memory_->ReadFully(UINT64_MAX - 100, buffer.data(), 200)); } -TEST_F(MemoryBufferTest, Read) { - ASSERT_TRUE(memory_->Resize(256)); +TEST_F(MemoryBufferTest, read_checks) { + memory_.reset(new MemoryBuffer(256)); ASSERT_EQ(256U, memory_->Size()); ASSERT_TRUE(memory_->GetPtr(0) != nullptr); ASSERT_TRUE(memory_->GetPtr(1) != nullptr); @@ -99,19 +98,43 @@ TEST_F(MemoryBufferTest, Read) { } } -TEST_F(MemoryBufferTest, Resize) { - ASSERT_TRUE(memory_->Resize(256)); - - ASSERT_TRUE(memory_->Resize(1024)); +TEST_F(MemoryBufferTest, resize_too_large) { + memory_.reset(new MemoryBuffer(SIZE_MAX)); + ASSERT_EQ(0U, memory_->Size()); } -extern "C" void __hwasan_init() __attribute__((weak)); +TEST_F(MemoryBufferTest, read_checks_non_zero_offset) { + memory_.reset(new MemoryBuffer(256, 1000)); + ASSERT_EQ(256U, memory_->Size()); + ASSERT_TRUE(memory_->GetPtr(0) == nullptr); + ASSERT_TRUE(memory_->GetPtr(1) == nullptr); + ASSERT_TRUE(memory_->GetPtr(999) == nullptr); + ASSERT_TRUE(memory_->GetPtr(1000) != nullptr); + ASSERT_TRUE(memory_->GetPtr(1255) != nullptr); + ASSERT_TRUE(memory_->GetPtr(1256) == nullptr); + + uint8_t* data = memory_->GetPtr(1000); + for (size_t i = 0; i < memory_->Size(); i++) { + data[i] = i; + } -TEST_F(MemoryBufferTest, Resize_too_large) { - if (&__hwasan_init != 0) { - GTEST_SKIP() << "Tests fails hwasan allocation size too large check."; + std::vector buffer(memory_->Size()); + ASSERT_EQ(128U, memory_->Read(1128, buffer.data(), buffer.size())); + for (size_t i = 0; i < 128; i++) { + ASSERT_EQ(128 + i, buffer[i]) << "Failed at byte " << i; } - ASSERT_FALSE(memory_->Resize(SIZE_MAX)); +} + +TEST_F(MemoryBufferTest, reads_non_zero_offset) { + memory_.reset(new MemoryBuffer(256, 1000)); + ASSERT_EQ(256U, memory_->Size()); + std::vector buffer(256); + ASSERT_EQ(0U, memory_->Read(900, buffer.data(), buffer.size())); + ASSERT_EQ(0U, memory_->Read(999, buffer.data(), buffer.size())); + ASSERT_EQ(256U, memory_->Read(1000, buffer.data(), buffer.size())); + ASSERT_EQ(246U, memory_->Read(1010, buffer.data(), buffer.size())); + ASSERT_EQ(1U, memory_->Read(1255, buffer.data(), buffer.size())); + ASSERT_EQ(0U, memory_->Read(1256, buffer.data(), buffer.size())); } } // namespace unwindstack diff --git a/libunwindstack/tests/MemoryXzTest.cpp b/libunwindstack/tests/MemoryXzTest.cpp index 17562dc..6597240 100644 --- a/libunwindstack/tests/MemoryXzTest.cpp +++ b/libunwindstack/tests/MemoryXzTest.cpp @@ -32,8 +32,8 @@ class MemoryXzTest : public ::testing::Test { std::string data; // NB: It is actually binary data. EXPECT_TRUE(android::base::ReadFileToString(dir + filename, &data)) << filename; EXPECT_GT(data.size(), 0u); - auto memory = std::make_unique(); - EXPECT_TRUE(memory->Resize(data.size())); + auto memory = std::make_unique(data.size()); + EXPECT_EQ(data.size(), memory->Size()); memcpy(memory->GetPtr(0), data.data(), data.size()); return memory; } -- cgit v1.2.3