From f447c8eb205d899085968a0a8dfae861ef56a589 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Mon, 3 Apr 2017 12:39:47 -0700 Subject: [PATCH] Add overflow checks in Memory objects. Also change one of the reads to be explicitly ReadField instead of an overloaded Read function. Bug: 23762183 Test: Passes new unit tests. Change-Id: Id848f7b632f67df0c5b7318d9e588942cfd2099a --- libunwindstack/ElfInterface.cpp | 34 +++--- libunwindstack/ElfInterfaceArm.cpp | 12 ++- libunwindstack/Memory.cpp | 59 ++++++++-- libunwindstack/Memory.h | 28 +++-- libunwindstack/tests/MemoryBuffer.cpp | 77 +++++++++++++ libunwindstack/tests/MemoryFake.h | 12 +++ libunwindstack/tests/MemoryFileTest.cpp | 57 +++++----- libunwindstack/tests/MemoryLocalTest.cpp | 29 ++--- libunwindstack/tests/MemoryRangeTest.cpp | 36 ++----- libunwindstack/tests/MemoryRemoteTest.cpp | 13 +++ libunwindstack/tests/MemoryTest.cpp | 126 ++++++++++++++++++++++ 11 files changed, 364 insertions(+), 119 deletions(-) create mode 100644 libunwindstack/tests/MemoryBuffer.cpp create mode 100644 libunwindstack/tests/MemoryTest.cpp diff --git a/libunwindstack/ElfInterface.cpp b/libunwindstack/ElfInterface.cpp index d59e9d8e3..087457c02 100644 --- a/libunwindstack/ElfInterface.cpp +++ b/libunwindstack/ElfInterface.cpp @@ -42,7 +42,7 @@ bool ElfInterface::ReadProgramHeaders(const EhdrType& ehdr) { uint64_t offset = ehdr.e_phoff; for (size_t i = 0; i < ehdr.e_phnum; i++, offset += ehdr.e_phentsize) { PhdrType phdr; - if (!memory_->Read(offset, &phdr, &phdr.p_type, sizeof(phdr.p_type))) { + if (!memory_->ReadField(offset, &phdr, &phdr.p_type, sizeof(phdr.p_type))) { return false; } @@ -54,20 +54,20 @@ bool ElfInterface::ReadProgramHeaders(const EhdrType& ehdr) { case PT_LOAD: { // Get the flags first, if this isn't an executable header, ignore it. - if (!memory_->Read(offset, &phdr, &phdr.p_flags, sizeof(phdr.p_flags))) { + if (!memory_->ReadField(offset, &phdr, &phdr.p_flags, sizeof(phdr.p_flags))) { return false; } if ((phdr.p_flags & PF_X) == 0) { continue; } - if (!memory_->Read(offset, &phdr, &phdr.p_vaddr, sizeof(phdr.p_vaddr))) { + if (!memory_->ReadField(offset, &phdr, &phdr.p_vaddr, sizeof(phdr.p_vaddr))) { return false; } - if (!memory_->Read(offset, &phdr, &phdr.p_offset, sizeof(phdr.p_offset))) { + if (!memory_->ReadField(offset, &phdr, &phdr.p_offset, sizeof(phdr.p_offset))) { return false; } - if (!memory_->Read(offset, &phdr, &phdr.p_memsz, sizeof(phdr.p_memsz))) { + if (!memory_->ReadField(offset, &phdr, &phdr.p_memsz, sizeof(phdr.p_memsz))) { return false; } pt_loads_[phdr.p_offset] = LoadInfo{phdr.p_offset, phdr.p_vaddr, @@ -79,22 +79,22 @@ bool ElfInterface::ReadProgramHeaders(const EhdrType& ehdr) { } case PT_GNU_EH_FRAME: - if (!memory_->Read(offset, &phdr, &phdr.p_offset, sizeof(phdr.p_offset))) { + if (!memory_->ReadField(offset, &phdr, &phdr.p_offset, sizeof(phdr.p_offset))) { return false; } eh_frame_offset_ = phdr.p_offset; - if (!memory_->Read(offset, &phdr, &phdr.p_memsz, sizeof(phdr.p_memsz))) { + if (!memory_->ReadField(offset, &phdr, &phdr.p_memsz, sizeof(phdr.p_memsz))) { return false; } eh_frame_size_ = phdr.p_memsz; break; case PT_DYNAMIC: - if (!memory_->Read(offset, &phdr, &phdr.p_offset, sizeof(phdr.p_offset))) { + if (!memory_->ReadField(offset, &phdr, &phdr.p_offset, sizeof(phdr.p_offset))) { return false; } dynamic_offset_ = phdr.p_offset; - if (!memory_->Read(offset, &phdr, &phdr.p_memsz, sizeof(phdr.p_memsz))) { + if (!memory_->ReadField(offset, &phdr, &phdr.p_memsz, sizeof(phdr.p_memsz))) { return false; } dynamic_size_ = phdr.p_memsz; @@ -116,8 +116,8 @@ bool ElfInterface::ReadSectionHeaders(const EhdrType& ehdr) { ShdrType shdr; if (ehdr.e_shstrndx < ehdr.e_shnum) { uint64_t sh_offset = offset + ehdr.e_shstrndx * ehdr.e_shentsize; - if (memory_->Read(sh_offset, &shdr, &shdr.sh_offset, sizeof(shdr.sh_offset)) - && memory_->Read(sh_offset, &shdr, &shdr.sh_size, sizeof(shdr.sh_size))) { + if (memory_->ReadField(sh_offset, &shdr, &shdr.sh_offset, sizeof(shdr.sh_offset)) && + memory_->ReadField(sh_offset, &shdr, &shdr.sh_size, sizeof(shdr.sh_size))) { sec_offset = shdr.sh_offset; sec_size = shdr.sh_size; } @@ -125,27 +125,27 @@ bool ElfInterface::ReadSectionHeaders(const EhdrType& ehdr) { // Skip the first header, it's always going to be NULL. for (size_t i = 1; i < ehdr.e_shnum; i++, offset += ehdr.e_shentsize) { - if (!memory_->Read(offset, &shdr, &shdr.sh_type, sizeof(shdr.sh_type))) { + if (!memory_->ReadField(offset, &shdr, &shdr.sh_type, sizeof(shdr.sh_type))) { return false; } if (shdr.sh_type == SHT_PROGBITS) { // Look for the .debug_frame and .gnu_debugdata. - if (!memory_->Read(offset, &shdr, &shdr.sh_name, sizeof(shdr.sh_name))) { + if (!memory_->ReadField(offset, &shdr, &shdr.sh_name, sizeof(shdr.sh_name))) { return false; } if (shdr.sh_name < sec_size) { std::string name; if (memory_->ReadString(sec_offset + shdr.sh_name, &name)) { if (name == ".debug_frame") { - if (memory_->Read(offset, &shdr, &shdr.sh_offset, sizeof(shdr.sh_offset)) - && memory_->Read(offset, &shdr, &shdr.sh_size, sizeof(shdr.sh_size))) { + if (memory_->ReadField(offset, &shdr, &shdr.sh_offset, sizeof(shdr.sh_offset)) && + memory_->ReadField(offset, &shdr, &shdr.sh_size, sizeof(shdr.sh_size))) { debug_frame_offset_ = shdr.sh_offset; debug_frame_size_ = shdr.sh_size; } } else if (name == ".gnu_debugdata") { - if (memory_->Read(offset, &shdr, &shdr.sh_offset, sizeof(shdr.sh_offset)) - && memory_->Read(offset, &shdr, &shdr.sh_size, sizeof(shdr.sh_size))) { + if (memory_->ReadField(offset, &shdr, &shdr.sh_offset, sizeof(shdr.sh_offset)) && + memory_->ReadField(offset, &shdr, &shdr.sh_size, sizeof(shdr.sh_size))) { gnu_debugdata_offset_ = shdr.sh_offset; gnu_debugdata_size_ = shdr.sh_size; } diff --git a/libunwindstack/ElfInterfaceArm.cpp b/libunwindstack/ElfInterfaceArm.cpp index e15732073..bab84ccb9 100644 --- a/libunwindstack/ElfInterfaceArm.cpp +++ b/libunwindstack/ElfInterfaceArm.cpp @@ -85,10 +85,10 @@ bool ElfInterfaceArm::HandleType(uint64_t offset, uint32_t type) { } Elf32_Phdr phdr; - if (!memory_->Read(offset, &phdr, &phdr.p_vaddr, sizeof(phdr.p_vaddr))) { + if (!memory_->ReadField(offset, &phdr, &phdr.p_vaddr, sizeof(phdr.p_vaddr))) { return true; } - if (!memory_->Read(offset, &phdr, &phdr.p_memsz, sizeof(phdr.p_memsz))) { + if (!memory_->ReadField(offset, &phdr, &phdr.p_memsz, sizeof(phdr.p_memsz))) { return true; } // The load_bias_ should always be set by this time. @@ -98,13 +98,15 @@ bool ElfInterfaceArm::HandleType(uint64_t offset, uint32_t type) { } bool ElfInterfaceArm::Step(uint64_t pc, Regs* regs, Memory* process_memory) { - return StepExidx(pc, regs, process_memory) || - ElfInterface32::Step(pc, regs, process_memory); + // Dwarf unwind information is precise about whether a pc is covered or not, + // but arm unwind information only has ranges of pc. In order to avoid + // incorrectly doing a bad unwind using arm unwind information for a + // different function, always try and unwind with the dwarf information first. + return ElfInterface32::Step(pc, regs, process_memory) || StepExidx(pc, regs, process_memory); } bool ElfInterfaceArm::StepExidx(uint64_t pc, Regs* regs, Memory* process_memory) { RegsArm* regs_arm = reinterpret_cast(regs); - // First try arm, then try dwarf. uint64_t entry_offset; if (!FindEntry(pc, &entry_offset)) { return false; diff --git a/libunwindstack/Memory.cpp b/libunwindstack/Memory.cpp index 1fcf84280..9e4650998 100644 --- a/libunwindstack/Memory.cpp +++ b/libunwindstack/Memory.cpp @@ -96,10 +96,16 @@ bool MemoryFileAtOffset::Init(const std::string& file, uint64_t offset, uint64_t offset_ = offset & (getpagesize() - 1); uint64_t aligned_offset = offset & ~(getpagesize() - 1); + if (aligned_offset > static_cast(buf.st_size) || + offset > static_cast(buf.st_size)) { + return false; + } + size_ = buf.st_size - aligned_offset; - if (size < (UINT64_MAX - offset_) && size + offset_ < size_) { + uint64_t max_size; + if (!__builtin_add_overflow(size, offset_, &max_size) && max_size < size_) { // Truncate the mapped size. - size_ = size + offset_; + size_ = max_size; } void* map = mmap(nullptr, size_, PROT_READ, MAP_PRIVATE, fd, aligned_offset); if (map == MAP_FAILED) { @@ -113,14 +119,15 @@ bool MemoryFileAtOffset::Init(const std::string& file, uint64_t offset, uint64_t } bool MemoryFileAtOffset::Read(uint64_t addr, void* dst, size_t size) { - if (addr + size > size_) { + uint64_t max_size; + if (__builtin_add_overflow(addr, size, &max_size) || max_size > size_) { return false; } memcpy(dst, &data_[addr], size); return true; } -static bool PtraceRead(pid_t pid, uint64_t addr, long* value) { +bool MemoryRemote::PtraceRead(uint64_t addr, long* value) { #if !defined(__LP64__) // Cannot read an address greater than 32 bits. if (addr > UINT32_MAX) { @@ -130,7 +137,7 @@ static bool PtraceRead(pid_t pid, uint64_t addr, long* value) { // ptrace() returns -1 and sets errno when the operation fails. // To disambiguate -1 from a valid result, we clear errno beforehand. errno = 0; - *value = ptrace(PTRACE_PEEKTEXT, pid, reinterpret_cast(addr), nullptr); + *value = ptrace(PTRACE_PEEKTEXT, pid_, reinterpret_cast(addr), nullptr); if (*value == -1 && errno) { return false; } @@ -138,11 +145,17 @@ static bool PtraceRead(pid_t pid, uint64_t addr, long* value) { } bool MemoryRemote::Read(uint64_t addr, void* dst, size_t bytes) { + // Make sure that there is no overflow. + uint64_t max_size; + if (__builtin_add_overflow(addr, bytes, &max_size)) { + return false; + } + size_t bytes_read = 0; long data; size_t align_bytes = addr & (sizeof(long) - 1); if (align_bytes != 0) { - if (!PtraceRead(pid_, addr & ~(sizeof(long) - 1), &data)) { + if (!PtraceRead(addr & ~(sizeof(long) - 1), &data)) { return false; } size_t copy_bytes = std::min(sizeof(long) - align_bytes, bytes); @@ -154,7 +167,7 @@ bool MemoryRemote::Read(uint64_t addr, void* dst, size_t bytes) { } for (size_t i = 0; i < bytes / sizeof(long); i++) { - if (!PtraceRead(pid_, addr, &data)) { + if (!PtraceRead(addr, &data)) { return false; } memcpy(dst, &data, sizeof(long)); @@ -165,7 +178,7 @@ bool MemoryRemote::Read(uint64_t addr, void* dst, size_t bytes) { size_t left_over = bytes & (sizeof(long) - 1); if (left_over) { - if (!PtraceRead(pid_, addr, &data)) { + if (!PtraceRead(addr, &data)) { return false; } memcpy(dst, &data, left_over); @@ -175,7 +188,13 @@ bool MemoryRemote::Read(uint64_t addr, void* dst, size_t bytes) { } bool MemoryLocal::Read(uint64_t addr, void* dst, size_t size) { - // The process_vm_readv call does will not always work on remote + // Make sure that there is no overflow. + uint64_t max_size; + if (__builtin_add_overflow(addr, size, &max_size)) { + return false; + } + + // The process_vm_readv call will not always work on remote // processes, so only use it for reads from the current pid. // Use this method to avoid crashes if an address is invalid since // unwind data could try to access any part of the address space. @@ -208,9 +227,29 @@ bool MemoryOffline::Init(const std::string& file, uint64_t offset) { } bool MemoryOffline::Read(uint64_t addr, void* dst, size_t size) { - if (addr < start_ || addr + size > start_ + offset_ + size_) { + uint64_t max_size; + if (__builtin_add_overflow(addr, size, &max_size)) { + return false; + } + + uint64_t real_size; + if (__builtin_add_overflow(start_, offset_, &real_size) || + __builtin_add_overflow(real_size, size_, &real_size)) { + return false; + } + + if (addr < start_ || max_size > real_size) { return false; } memcpy(dst, &data_[addr + offset_ - start_ + sizeof(start_)], size); return true; } + +bool MemoryRange::Read(uint64_t addr, void* dst, size_t size) { + uint64_t max_read; + if (__builtin_add_overflow(addr, size, &max_read) || max_read > length_) { + return false; + } + // The check above guarantees that addr + begin_ will not overflow. + return memory_->Read(addr + begin_, dst, size); +} diff --git a/libunwindstack/Memory.h b/libunwindstack/Memory.h index c5316a1d0..f9f6d5662 100644 --- a/libunwindstack/Memory.h +++ b/libunwindstack/Memory.h @@ -17,6 +17,7 @@ #ifndef _LIBUNWINDSTACK_MEMORY_H #define _LIBUNWINDSTACK_MEMORY_H +#include #include #include #include @@ -33,9 +34,16 @@ class Memory { virtual bool Read(uint64_t addr, void* dst, size_t size) = 0; - inline bool Read(uint64_t addr, void* start, void* field, size_t size) { - return Read(addr + reinterpret_cast(field) - reinterpret_cast(start), - field, size); + inline bool ReadField(uint64_t addr, void* start, void* field, size_t size) { + if (reinterpret_cast(field) < reinterpret_cast(start)) { + return false; + } + uint64_t offset = reinterpret_cast(field) - reinterpret_cast(start); + if (__builtin_add_overflow(addr, offset, &offset)) { + return false; + } + // The read will check if offset + size overflows. + return Read(offset, field, size); } inline bool Read32(uint64_t addr, uint32_t* dst) { @@ -103,6 +111,9 @@ class MemoryRemote : public Memory { pid_t pid() { return pid_; } + protected: + virtual bool PtraceRead(uint64_t addr, long* value); + private: pid_t pid_; }; @@ -118,15 +129,12 @@ class MemoryLocal : public Memory { class MemoryRange : public Memory { public: MemoryRange(Memory* memory, uint64_t begin, uint64_t end) - : memory_(memory), begin_(begin), length_(end - begin_) {} + : memory_(memory), begin_(begin), length_(end - begin) { + assert(end > begin); + } virtual ~MemoryRange() { delete memory_; } - inline bool Read(uint64_t addr, void* dst, size_t size) override { - if (addr + size <= length_) { - return memory_->Read(addr + begin_, dst, size); - } - return false; - } + bool Read(uint64_t addr, void* dst, size_t size) override; private: Memory* memory_; diff --git a/libunwindstack/tests/MemoryBuffer.cpp b/libunwindstack/tests/MemoryBuffer.cpp new file mode 100644 index 000000000..af3d6b9ae --- /dev/null +++ b/libunwindstack/tests/MemoryBuffer.cpp @@ -0,0 +1,77 @@ +/* + * Copyright (C) 2016 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include + +#include + +#include "Memory.h" + +#include "LogFake.h" + +class MemoryBufferTest : public ::testing::Test { + protected: + void SetUp() override { + ResetLogs(); + memory_.reset(new MemoryBuffer); + } + std::unique_ptr memory_; +}; + +TEST_F(MemoryBufferTest, empty) { + ASSERT_EQ(0U, memory_->Size()); + std::vector buffer(1024); + ASSERT_FALSE(memory_->Read(0, buffer.data(), 1)); + ASSERT_EQ(nullptr, memory_->GetPtr(0)); + ASSERT_EQ(nullptr, memory_->GetPtr(1)); +} + +TEST_F(MemoryBufferTest, write_read) { + memory_->Resize(256); + ASSERT_EQ(256U, memory_->Size()); + ASSERT_TRUE(memory_->GetPtr(0) != nullptr); + ASSERT_TRUE(memory_->GetPtr(1) != nullptr); + ASSERT_TRUE(memory_->GetPtr(255) != nullptr); + ASSERT_TRUE(memory_->GetPtr(256) == nullptr); + + uint8_t* data = memory_->GetPtr(0); + for (size_t i = 0; i < memory_->Size(); i++) { + data[i] = i; + } + + std::vector buffer(memory_->Size()); + ASSERT_TRUE(memory_->Read(0, buffer.data(), buffer.size())); + for (size_t i = 0; i < buffer.size(); i++) { + ASSERT_EQ(i, buffer[i]) << "Failed at byte " << i; + } +} + +TEST_F(MemoryBufferTest, read_failures) { + memory_->Resize(100); + std::vector buffer(200); + ASSERT_FALSE(memory_->Read(0, buffer.data(), 101)); + ASSERT_FALSE(memory_->Read(100, buffer.data(), 1)); + ASSERT_FALSE(memory_->Read(101, buffer.data(), 2)); + ASSERT_FALSE(memory_->Read(99, buffer.data(), 2)); + ASSERT_TRUE(memory_->Read(99, buffer.data(), 1)); +} + +TEST_F(MemoryBufferTest, read_failure_overflow) { + memory_->Resize(100); + std::vector buffer(200); + + ASSERT_FALSE(memory_->Read(UINT64_MAX - 100, buffer.data(), 200)); +} diff --git a/libunwindstack/tests/MemoryFake.h b/libunwindstack/tests/MemoryFake.h index e05736bec..70ef30aaf 100644 --- a/libunwindstack/tests/MemoryFake.h +++ b/libunwindstack/tests/MemoryFake.h @@ -75,4 +75,16 @@ class MemoryFakeAlwaysReadZero : public Memory { } }; +class MemoryFakeRemote : public MemoryRemote { + public: + MemoryFakeRemote() : MemoryRemote(0) {} + virtual ~MemoryFakeRemote() = default; + + protected: + bool PtraceRead(uint64_t, long* value) override { + *value = 0; + return true; + } +}; + #endif // _LIBUNWINDSTACK_TESTS_MEMORY_FAKE_H diff --git a/libunwindstack/tests/MemoryFileTest.cpp b/libunwindstack/tests/MemoryFileTest.cpp index 870ca19d2..aa7a23a34 100644 --- a/libunwindstack/tests/MemoryFileTest.cpp +++ b/libunwindstack/tests/MemoryFileTest.cpp @@ -14,6 +14,9 @@ * limitations under the License. */ +#include +#include + #include #include #include @@ -39,7 +42,7 @@ class MemoryFileTest : public ::testing::Test { TemporaryFile* tf_ = nullptr; }; -TEST_F(MemoryFileTest, offset_0) { +TEST_F(MemoryFileTest, init_offset_0) { WriteTestData(); ASSERT_TRUE(memory_.Init(tf_->path, 0)); @@ -49,7 +52,7 @@ TEST_F(MemoryFileTest, offset_0) { ASSERT_STREQ("0123456789", buffer.data()); } -TEST_F(MemoryFileTest, offset_non_zero) { +TEST_F(MemoryFileTest, init_offset_non_zero) { WriteTestData(); ASSERT_TRUE(memory_.Init(tf_->path, 10)); @@ -59,7 +62,7 @@ TEST_F(MemoryFileTest, offset_non_zero) { ASSERT_STREQ("abcdefghij", buffer.data()); } -TEST_F(MemoryFileTest, offset_non_zero_larger_than_pagesize) { +TEST_F(MemoryFileTest, init_offset_non_zero_larger_than_pagesize) { size_t pagesize = getpagesize(); std::string large_string; for (size_t i = 0; i < pagesize; i++) { @@ -75,7 +78,7 @@ TEST_F(MemoryFileTest, offset_non_zero_larger_than_pagesize) { ASSERT_STREQ("abcdefgh", buffer.data()); } -TEST_F(MemoryFileTest, offset_pagesize_aligned) { +TEST_F(MemoryFileTest, init_offset_pagesize_aligned) { size_t pagesize = getpagesize(); std::string data; for (size_t i = 0; i < 2 * pagesize; i++) { @@ -96,7 +99,7 @@ TEST_F(MemoryFileTest, offset_pagesize_aligned) { ASSERT_STREQ(expected_str.c_str(), buffer.data()); } -TEST_F(MemoryFileTest, offset_pagesize_aligned_plus_extra) { +TEST_F(MemoryFileTest, init_offset_pagesize_aligned_plus_extra) { size_t pagesize = getpagesize(); std::string data; for (size_t i = 0; i < 2 * pagesize; i++) { @@ -117,6 +120,23 @@ TEST_F(MemoryFileTest, offset_pagesize_aligned_plus_extra) { ASSERT_STREQ(expected_str.c_str(), buffer.data()); } +TEST_F(MemoryFileTest, init_offset_greater_than_filesize) { + size_t pagesize = getpagesize(); + std::string data; + uint64_t file_size = 2 * pagesize + pagesize / 2; + for (size_t i = 0; i < file_size; i++) { + data += static_cast((i / pagesize) + '0'); + } + ASSERT_TRUE(android::base::WriteStringToFd(data, tf_->fd)); + + // Check offset > file size fails and aligned_offset > file size. + ASSERT_FALSE(memory_.Init(tf_->path, file_size + 2 * pagesize)); + // Check offset == filesize fails. + ASSERT_FALSE(memory_.Init(tf_->path, file_size)); + // Check aligned_offset < filesize, but offset > filesize fails. + ASSERT_FALSE(memory_.Init(tf_->path, 2 * pagesize + pagesize / 2 + pagesize / 4)); +} + TEST_F(MemoryFileTest, read_error) { std::string data; for (size_t i = 0; i < 5000; i++) { @@ -137,32 +157,9 @@ TEST_F(MemoryFileTest, read_error) { ASSERT_TRUE(memory_.Read(4990, buffer.data(), 10)); ASSERT_FALSE(memory_.Read(4999, buffer.data(), 2)); ASSERT_TRUE(memory_.Read(4999, buffer.data(), 1)); -} -TEST_F(MemoryFileTest, read_string) { - std::string value("name_in_file"); - ASSERT_TRUE(android::base::WriteFully(tf_->fd, value.c_str(), value.size() + 1)); - - std::string name; - ASSERT_TRUE(memory_.Init(tf_->path, 0)); - ASSERT_TRUE(memory_.ReadString(0, &name)); - ASSERT_EQ("name_in_file", name); - ASSERT_TRUE(memory_.ReadString(5, &name)); - ASSERT_EQ("in_file", name); -} - -TEST_F(MemoryFileTest, read_string_error) { - std::vector buffer = { 0x23, 0x32, 0x45 }; - ASSERT_TRUE(android::base::WriteFully(tf_->fd, buffer.data(), buffer.size())); - - std::string name; - ASSERT_TRUE(memory_.Init(tf_->path, 0)); - - // Read from a non-existant address. - ASSERT_FALSE(memory_.ReadString(100, &name)); - - // This should fail because there is no terminating \0 - ASSERT_FALSE(memory_.ReadString(0, &name)); + // Check that overflow fails properly. + ASSERT_FALSE(memory_.Read(UINT64_MAX - 100, buffer.data(), 200)); } TEST_F(MemoryFileTest, read_past_file_within_mapping) { diff --git a/libunwindstack/tests/MemoryLocalTest.cpp b/libunwindstack/tests/MemoryLocalTest.cpp index 0ba5f1c37..ab999dafc 100644 --- a/libunwindstack/tests/MemoryLocalTest.cpp +++ b/libunwindstack/tests/MemoryLocalTest.cpp @@ -47,25 +47,6 @@ TEST(MemoryLocalTest, read) { } } -TEST(MemoryLocalTest, read_string) { - std::string name("string_in_memory"); - - MemoryLocal local; - - std::vector dst(1024); - std::string dst_name; - ASSERT_TRUE(local.ReadString(reinterpret_cast(name.c_str()), &dst_name)); - ASSERT_EQ("string_in_memory", dst_name); - - ASSERT_TRUE(local.ReadString(reinterpret_cast(&name[7]), &dst_name)); - ASSERT_EQ("in_memory", dst_name); - - ASSERT_TRUE(local.ReadString(reinterpret_cast(&name[7]), &dst_name, 10)); - ASSERT_EQ("in_memory", dst_name); - - ASSERT_FALSE(local.ReadString(reinterpret_cast(&name[7]), &dst_name, 9)); -} - TEST(MemoryLocalTest, read_illegal) { MemoryLocal local; @@ -73,3 +54,13 @@ TEST(MemoryLocalTest, read_illegal) { ASSERT_FALSE(local.Read(0, dst.data(), 1)); ASSERT_FALSE(local.Read(0, dst.data(), 100)); } + +TEST(MemoryLocalTest, read_overflow) { + MemoryLocal local; + + // On 32 bit this test doesn't necessarily cause an overflow. The 64 bit + // version will always go through the overflow check. + std::vector dst(100); + uint64_t value; + ASSERT_FALSE(local.Read(reinterpret_cast(&value), dst.data(), SIZE_MAX)); +} diff --git a/libunwindstack/tests/MemoryRangeTest.cpp b/libunwindstack/tests/MemoryRangeTest.cpp index d636ec497..ee5ba018b 100644 --- a/libunwindstack/tests/MemoryRangeTest.cpp +++ b/libunwindstack/tests/MemoryRangeTest.cpp @@ -17,6 +17,7 @@ #include #include +#include #include #include @@ -65,35 +66,14 @@ TEST_F(MemoryRangeTest, read_near_limit) { ASSERT_FALSE(range.Read(1020, dst.data(), 5)); ASSERT_FALSE(range.Read(1024, dst.data(), 1)); ASSERT_FALSE(range.Read(1024, dst.data(), 1024)); + + // Verify that reading up to the end works. + ASSERT_TRUE(range.Read(1020, dst.data(), 4)); } -TEST_F(MemoryRangeTest, read_string_past_end) { - std::string name("0123456789"); - memory_->SetMemory(0, name); +TEST_F(MemoryRangeTest, read_overflow) { + std::vector buffer(100); - // Verify a read past the range fails. - MemoryRange range(memory_, 0, 5); - std::string dst_name; - ASSERT_FALSE(range.ReadString(0, &dst_name)); -} - -TEST_F(MemoryRangeTest, read_string_to_end) { - std::string name("0123456789"); - memory_->SetMemory(30, name); - - // Verify the range going to the end of the string works. - MemoryRange range(memory_, 30, 30 + name.size() + 1); - std::string dst_name; - ASSERT_TRUE(range.ReadString(0, &dst_name)); - ASSERT_EQ("0123456789", dst_name); -} - -TEST_F(MemoryRangeTest, read_string_fencepost) { - std::string name("0123456789"); - memory_->SetMemory(10, name); - - // Verify the range set to one byte less than the end of the string fails. - MemoryRange range(memory_, 10, 10 + name.size()); - std::string dst_name; - ASSERT_FALSE(range.ReadString(0, &dst_name)); + std::unique_ptr overflow(new MemoryRange(new MemoryFakeAlwaysReadZero, 100, 200)); + ASSERT_FALSE(overflow->Read(UINT64_MAX - 10, buffer.data(), 100)); } diff --git a/libunwindstack/tests/MemoryRemoteTest.cpp b/libunwindstack/tests/MemoryRemoteTest.cpp index 7664c3e00..e48edf7e5 100644 --- a/libunwindstack/tests/MemoryRemoteTest.cpp +++ b/libunwindstack/tests/MemoryRemoteTest.cpp @@ -33,6 +33,8 @@ #include "Memory.h" +#include "MemoryFake.h" + class MemoryRemoteTest : public ::testing::Test { protected: static uint64_t NanoTime() { @@ -121,6 +123,9 @@ TEST_F(MemoryRemoteTest, read_fail) { ASSERT_TRUE(remote.Read(reinterpret_cast(src) + pagesize - 1, dst.data(), 1)); ASSERT_FALSE(remote.Read(reinterpret_cast(src) + pagesize - 4, dst.data(), 8)); + // Check overflow condition is caught properly. + ASSERT_FALSE(remote.Read(UINT64_MAX - 100, dst.data(), 200)); + ASSERT_EQ(0, munmap(src, pagesize)); ASSERT_TRUE(Detach(pid)); @@ -128,6 +133,14 @@ TEST_F(MemoryRemoteTest, read_fail) { kill(pid, SIGKILL); } +TEST_F(MemoryRemoteTest, read_overflow) { + MemoryFakeRemote remote; + + // Check overflow condition is caught properly. + std::vector dst(200); + ASSERT_FALSE(remote.Read(UINT64_MAX - 100, dst.data(), 200)); +} + TEST_F(MemoryRemoteTest, read_illegal) { pid_t pid; if ((pid = fork()) == 0) { diff --git a/libunwindstack/tests/MemoryTest.cpp b/libunwindstack/tests/MemoryTest.cpp new file mode 100644 index 000000000..51b5d7db7 --- /dev/null +++ b/libunwindstack/tests/MemoryTest.cpp @@ -0,0 +1,126 @@ +/* + * Copyright (C) 2017 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include + +#include +#include + +#include + +#include "Memory.h" + +#include "MemoryFake.h" + +TEST(MemoryTest, read32) { + MemoryFakeAlwaysReadZero memory; + + uint32_t data = 0xffffffff; + ASSERT_TRUE(memory.Read32(0, &data)); + ASSERT_EQ(0U, data); +} + +TEST(MemoryTest, read64) { + MemoryFakeAlwaysReadZero memory; + + uint64_t data = 0xffffffffffffffffULL; + ASSERT_TRUE(memory.Read64(0, &data)); + ASSERT_EQ(0U, data); +} + +struct FakeStruct { + int one; + bool two; + uint32_t three; + uint64_t four; +}; + +TEST(MemoryTest, read_field) { + MemoryFakeAlwaysReadZero memory; + + FakeStruct data; + memset(&data, 0xff, sizeof(data)); + ASSERT_TRUE(memory.ReadField(0, &data, &data.one, sizeof(data.one))); + ASSERT_EQ(0, data.one); + + memset(&data, 0xff, sizeof(data)); + ASSERT_TRUE(memory.ReadField(0, &data, &data.two, sizeof(data.two))); + ASSERT_FALSE(data.two); + + memset(&data, 0xff, sizeof(data)); + ASSERT_TRUE(memory.ReadField(0, &data, &data.three, sizeof(data.three))); + ASSERT_EQ(0U, data.three); + + memset(&data, 0xff, sizeof(data)); + ASSERT_TRUE(memory.ReadField(0, &data, &data.four, sizeof(data.four))); + ASSERT_EQ(0U, data.four); +} + +TEST(MemoryTest, read_field_fails) { + MemoryFakeAlwaysReadZero memory; + + FakeStruct data; + memset(&data, 0xff, sizeof(data)); + + ASSERT_FALSE(memory.ReadField(UINT64_MAX, &data, &data.three, sizeof(data.three))); + + // Field and start reversed, should fail. + ASSERT_FALSE(memory.ReadField(100, &data.two, &data, sizeof(data.two))); + ASSERT_FALSE(memory.ReadField(0, &data.two, &data, sizeof(data.two))); +} + +TEST(MemoryTest, read_string) { + std::string name("string_in_memory"); + + MemoryFake memory; + + memory.SetMemory(100, name.c_str(), name.size() + 1); + + std::string dst_name; + ASSERT_TRUE(memory.ReadString(100, &dst_name)); + ASSERT_EQ("string_in_memory", dst_name); + + ASSERT_TRUE(memory.ReadString(107, &dst_name)); + ASSERT_EQ("in_memory", dst_name); + + // Set size greater than string. + ASSERT_TRUE(memory.ReadString(107, &dst_name, 10)); + ASSERT_EQ("in_memory", dst_name); + + ASSERT_FALSE(memory.ReadString(107, &dst_name, 9)); +} + +TEST(MemoryTest, read_string_error) { + std::string name("short"); + + MemoryFake memory; + + // Save everything except the terminating '\0'. + memory.SetMemory(0, name.c_str(), name.size()); + + std::string dst_name; + // Read from a non-existant address. + ASSERT_FALSE(memory.ReadString(100, &dst_name)); + + // This should fail because there is no terminating '\0'. + ASSERT_FALSE(memory.ReadString(0, &dst_name)); + + // This should pass because there is a terminating '\0'. + memory.SetData8(name.size(), '\0'); + ASSERT_TRUE(memory.ReadString(0, &dst_name)); + ASSERT_EQ("short", dst_name); +}