[lldb] Guard MemoryCache::ReadRanges against a short buffer

[qiyao]

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.

[llvmorg-github-actions]

@llvm/pr-subscribers-lldb

Author: Yao Qi (qiyao)

Changes

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 --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());

[jasonmolenda]

Looks good to me, but this is Felipe's code so he's the important one to review.