llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-lldb
Author: Yao Qi (qiyao)
<details>
<summary>Changes</summary>
`Process::DoReadMemoryRanges` asserts the caller's buffer is long enough
before it writes into it, and returns empty ranges when asserts are off.
`MemoryCache::ReadRanges` writes a cache hit into the same buffer with no
such check, in `lldb/source/Target/Memory.cpp`:
```
316: results.push_back(buffer.take_front(len)); // take_front clamps 16
-> 8
317: buffer = buffer.drop_front(len); // asks to drop 16 from 8
318: memcpy(results.back().data(), cached, len); // writes 16 into 8 bytes
```
With assertions on, `MutableArrayRef::drop_front` aborts one line before
the `memcpy`, so the new test dies on the wrong message:
```
Death test: { read_results = cache.ReadRanges(ranges, short_buffer); }
Result: died but not with expected error.
Expected: contains regular expression "MemoryCache::ReadRanges: provided
buffer is too short"
Actual msg:
[ DEATH ] Assertion failed: (this->size() >= N && "Dropping
more elements than exist"), function drop_front, file ArrayRef.h, line 384.
```
With assertions off that abort is gone and the `memcpy` runs. An
AddressSanitizer build reports a heap-buffer-overflow write of 16 bytes
into the 8 byte buffer, 8 bytes past its end.
`Process::ReadMemoryRanges` goes through the cache unless it is disabled,
so the cache is the first of the two to write and it needs the same guard.
`TestReadMemoryRangesWithShortBuffer` did not catch this because both of
its ranges miss the cache, which leaves the whole buffer to
`DoReadMemoryRanges` and lets that assert fire first. Disable the cache
in it so it reaches the guard it is named after, and add a case whose
range hits the cache. Name the function in both assert messages: the old
message is a substring of the new one, and a death test matches by
substring, so naming only the new guard would leave that test accepting
either.
---
Full diff: https://github.com/llvm/llvm-project/pull/218706.diff
3 Files Affected:
- (modified) lldb/source/Target/Memory.cpp (+11)
- (modified) lldb/source/Target/Process.cpp (+2-1)
- (modified) lldb/unittests/Target/MemoryTest.cpp (+38-1)
``````````diff
diff --git a/lldb/source/Target/Memory.cpp b/lldb/source/Target/Memory.cpp
index 2cb5a920f6c66..223a21b802d94 100644
--- a/lldb/source/Target/Memory.cpp
+++ b/lldb/source/Target/Memory.cpp
@@ -295,6 +295,17 @@ size_t MemoryCache::Read(addr_t addr, void *dst, size_t
dst_len,
llvm::SmallVector<llvm::MutableArrayRef<uint8_t>>
MemoryCache::ReadRanges(llvm::ArrayRef<Range<lldb::addr_t, size_t>> ranges,
llvm::MutableArrayRef<uint8_t> buffer) {
+ // A cache hit writes into `buffer` below, so check its size before that
+ // write. Fail the same way Process::DoReadMemoryRanges does.
+ auto total_ranges_len = llvm::sum_of(
+ llvm::map_range(ranges, [](auto range) { return range.size; }));
+ assert(buffer.size() >= total_ranges_len &&
+ "MemoryCache::ReadRanges: provided buffer is too short");
+ if (buffer.size() < total_ranges_len) {
+ llvm::MutableArrayRef<uint8_t> empty;
+ return {ranges.size(), empty};
+ }
+
std::lock_guard<std::recursive_mutex> guard(m_mutex);
llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> results;
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 1d7e413603c82..fca16b955021b 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -2102,7 +2102,8 @@
Process::DoReadMemoryRanges(llvm::ArrayRef<Range<lldb::addr_t, size_t>> ranges,
// If the buffer is not large enough, this is a programmer error.
// In production builds, gracefully fail by returning a length of 0 for all
// ranges.
- assert(buffer.size() >= total_ranges_len && "provided buffer is too short");
+ assert(buffer.size() >= total_ranges_len &&
+ "Process::DoReadMemoryRanges: provided buffer is too short");
if (buffer.size() < total_ranges_len) {
llvm::MutableArrayRef<uint8_t> empty;
return {ranges.size(), empty};
diff --git a/lldb/unittests/Target/MemoryTest.cpp
b/lldb/unittests/Target/MemoryTest.cpp
index 464ae033eacee..4501de43b2d1a 100644
--- a/lldb/unittests/Target/MemoryTest.cpp
+++ b/lldb/unittests/Target/MemoryTest.cpp
@@ -691,6 +691,37 @@ TEST_F(MemoryDeathTest,
TestReadMemoryRangesReturnsTooMuch) {
#endif
}
+TEST_F(MemoryDeathTest, TestReadRangesWithShortBufferAndCacheHit) {
+ GTEST_FLAG_SET(death_test_style, "threadsafe");
+
+ ArchSpec arch("arm64-apple-macosx");
+ Platform::SetHostPlatform(PlatformRemoteMacOSX::CreateInstance(true, &arch));
+ DebuggerSP debugger_sp = Debugger::CreateInstance();
+ ASSERT_TRUE(debugger_sp);
+ TargetSP target_sp = CreateTarget(debugger_sp, arch);
+ ASSERT_TRUE(target_sp);
+ ProcessSP process_sp = CreateProcess(target_sp);
+ ASSERT_TRUE(process_sp);
+
+ DummyProcess *process = static_cast<DummyProcess *>(process_sp.get());
+ TestMemoryCache cache(*process);
+ cache.AddL1CacheData(0x1000, std::make_shared<DataBufferHeap>(16, 0xAA));
+ ASSERT_EQ(cache.GetL1Cache().count(0x1000), 1u);
+
+ llvm::SmallVector<uint8_t, 0> short_buffer(8, 0);
+ llvm::SmallVector<Range<addr_t, size_t>> ranges = {{0x1000, 16}};
+ llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> read_results;
+ ASSERT_DEBUG_DEATH(
+ { read_results = cache.ReadRanges(ranges, short_buffer); },
+ "MemoryCache::ReadRanges: provided buffer is too short");
+#ifdef NDEBUG
+ // With asserts off, the ranges come back empty instead.
+ ASSERT_EQ(read_results.size(), ranges.size());
+ for (llvm::MutableArrayRef<uint8_t> result : read_results)
+ ASSERT_TRUE(result.empty());
+#endif
+}
+
TEST_F(MemoryDeathTest, TestReadMemoryRangesWithShortBuffer) {
// gtest death-tests execute in a sub-process (fork), which invalidates
// any signpost handles and would cause spurious crashes if used. Use the
@@ -710,13 +741,19 @@ TEST_F(MemoryDeathTest,
TestReadMemoryRangesWithShortBuffer) {
std::make_shared<DummyReaderProcess>(target_sp, listener_sp);
ASSERT_TRUE(process_sp);
+ // Memory cache has to be off to reach the one in
Process::DoReadMemoryRanges.
+ Status set_error = process_sp->SetPropertyValue(
+ nullptr, eVarSetOperationAssign, "disable-memory-cache", "true");
+ ASSERT_TRUE(set_error.Success()) << set_error.AsCString();
+ ASSERT_TRUE(process_sp->GetDisableMemoryCache());
+
llvm::SmallVector<uint8_t, 0> short_buffer(10, 0);
llvm::SmallVector<Range<addr_t, size_t>> ranges = {{0x12345, 128},
{0x11, 128}};
llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> read_results;
ASSERT_DEBUG_DEATH(
{ read_results = process_sp->ReadMemoryRanges(ranges, short_buffer); },
- "provided buffer is too short");
+ "Process::DoReadMemoryRanges: provided buffer is too short");
#ifdef NDEBUG
// With asserts off, the read should return empty ranges.
ASSERT_EQ(read_results.size(), ranges.size());
``````````
</details>
https://github.com/llvm/llvm-project/pull/218706
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
