From 0ae4022aa0bb2572ef7b997d00904125a0e089b1 Mon Sep 17 00:00:00 2001 From: Pavel Labath Date: Wed, 26 Sep 2018 07:31:41 +0000 Subject: [PATCH] Fix a memory read bug in lldb-server NativeProcessProtocol::ReadMemoryWithoutTrap had a bug, where it failed to properly remove inserted breakpoint opcodes if the memory read partially overlapped the trap opcode. This could not happen on x86 because it has a one-byte breakpoint instruction, but it could happen on arm, which has a 4-byte breakpoint instruction (in arm mode). Since triggerring this condition would only be possible on an arm machine (and even then it would be a bit tricky). I test this using a NativeProcessProtocol unit test. llvm-svn: 343076 --- .../Host/common/NativeBreakpointList.cpp | 25 +++++-- .../Host/NativeProcessProtocolTest.cpp | 67 +++++++++++++++++++ 2 files changed, 85 insertions(+), 7 deletions(-) diff --git a/lldb/source/Host/common/NativeBreakpointList.cpp b/lldb/source/Host/common/NativeBreakpointList.cpp index cfcbe0831064..b1a42d7583c9 100644 --- a/lldb/source/Host/common/NativeBreakpointList.cpp +++ b/lldb/source/Host/common/NativeBreakpointList.cpp @@ -210,20 +210,31 @@ Status NativeBreakpointList::GetBreakpoint(lldb::addr_t addr, Status NativeBreakpointList::RemoveTrapsFromBuffer(lldb::addr_t addr, void *buf, size_t size) const { + auto data = llvm::makeMutableArrayRef(static_cast(buf), size); for (const auto &map : m_breakpoints) { - lldb::addr_t bp_addr = map.first; - // Breapoint not in range, ignore - if (bp_addr < addr || addr + size <= bp_addr) - continue; const auto &bp_sp = map.second; // Not software breakpoint, ignore if (!bp_sp->IsSoftwareBreakpoint()) continue; auto software_bp_sp = std::static_pointer_cast(bp_sp); - auto opcode_addr = static_cast(buf) + bp_addr - addr; - auto saved_opcodes = software_bp_sp->m_saved_opcodes; + + lldb::addr_t bp_addr = map.first; auto opcode_size = software_bp_sp->m_opcode_size; - ::memcpy(opcode_addr, saved_opcodes, opcode_size); + + // Breapoint not in range, ignore + if (bp_addr + opcode_size < addr || addr + size <= bp_addr) + continue; + + auto saved_opcodes = + llvm::makeArrayRef(software_bp_sp->m_saved_opcodes, opcode_size); + if (bp_addr < addr) { + saved_opcodes = saved_opcodes.drop_front(addr - bp_addr); + bp_addr = addr; + } + auto bp_data = data.drop_front(bp_addr - addr); + std::copy_n(saved_opcodes.begin(), + std::min(saved_opcodes.size(), bp_data.size()), + bp_data.begin()); } return Status(); } diff --git a/lldb/unittests/Host/NativeProcessProtocolTest.cpp b/lldb/unittests/Host/NativeProcessProtocolTest.cpp index 8b519fdf6aca..ff5d6371b5d8 100644 --- a/lldb/unittests/Host/NativeProcessProtocolTest.cpp +++ b/lldb/unittests/Host/NativeProcessProtocolTest.cpp @@ -70,10 +70,22 @@ public: llvm::ArrayRef Data)); using NativeProcessProtocol::GetSoftwareBreakpointTrapOpcode; + llvm::Expected> ReadMemoryWithoutTrap(addr_t Addr, + size_t Size); private: ArchSpec Arch; }; + +class FakeMemory { +public: + FakeMemory(llvm::ArrayRef Data) : Data(Data) {} + llvm::Expected> Read(addr_t Addr, size_t Size); + llvm::Expected Write(addr_t Addr, llvm::ArrayRef Chunk); + +private: + std::vector Data; +}; } // namespace Status MockProcess::ReadMemory(addr_t Addr, void *Buf, size_t Size, @@ -101,6 +113,37 @@ Status MockProcess::WriteMemory(addr_t Addr, const void *Buf, size_t Size, return Status(); } +llvm::Expected> +MockProcess::ReadMemoryWithoutTrap(addr_t Addr, size_t Size) { + std::vector Data(Size, 0); + size_t BytesRead; + Status ST = NativeProcessProtocol::ReadMemoryWithoutTrap( + Addr, Data.data(), Data.size(), BytesRead); + if (ST.Fail()) + return ST.ToError(); + Data.resize(BytesRead); + return std::move(Data); +} + +llvm::Expected> FakeMemory::Read(addr_t Addr, + size_t Size) { + if (Addr >= Data.size()) + return llvm::createStringError(llvm::inconvertibleErrorCode(), + "Address out of range."); + Size = std::min(Size, Data.size() - Addr); + return std::vector(&Data[Addr], &Data[Addr + Size]); +} + +llvm::Expected FakeMemory::Write(addr_t Addr, + llvm::ArrayRef Chunk) { + if (Addr >= Data.size()) + return llvm::createStringError(llvm::inconvertibleErrorCode(), + "Address out of range."); + size_t Size = std::min(Chunk.size(), Data.size() - Addr); + std::copy_n(Chunk.begin(), Size, &Data[Addr]); + return Size; +} + TEST(NativeProcessProtocolTest, SetBreakpoint) { NiceMock DummyDelegate; MockProcess Process(DummyDelegate, ArchSpec("x86_64-pc-linux")); @@ -152,3 +195,27 @@ TEST(NativeProcessProtocolTest, SetBreakpointFailVerify) { EXPECT_THAT_ERROR(Process.SetBreakpoint(0x47, 0, false).ToError(), llvm::Failed()); } + +TEST(NativeProcessProtocolTest, ReadMemoryWithoutTrap) { + NiceMock DummyDelegate; + MockProcess Process(DummyDelegate, ArchSpec("aarch64-pc-linux")); + FakeMemory M{{0, 1, 2, 3, 4, 5, 6, 7, 8, 9}}; + EXPECT_CALL(Process, ReadMemory(_, _)) + .WillRepeatedly(Invoke(&M, &FakeMemory::Read)); + EXPECT_CALL(Process, WriteMemory(_, _)) + .WillRepeatedly(Invoke(&M, &FakeMemory::Write)); + + EXPECT_THAT_ERROR(Process.SetBreakpoint(0x4, 0, false).ToError(), + llvm::Succeeded()); + EXPECT_THAT_EXPECTED( + Process.ReadMemoryWithoutTrap(0, 10), + llvm::HasValue(std::vector{0, 1, 2, 3, 4, 5, 6, 7, 8, 9})); + EXPECT_THAT_EXPECTED(Process.ReadMemoryWithoutTrap(0, 6), + llvm::HasValue(std::vector{0, 1, 2, 3, 4, 5})); + EXPECT_THAT_EXPECTED(Process.ReadMemoryWithoutTrap(6, 4), + llvm::HasValue(std::vector{6, 7, 8, 9})); + EXPECT_THAT_EXPECTED(Process.ReadMemoryWithoutTrap(6, 2), + llvm::HasValue(std::vector{6, 7})); + EXPECT_THAT_EXPECTED(Process.ReadMemoryWithoutTrap(4, 2), + llvm::HasValue(std::vector{4, 5})); +} -- 2.34.1