[lldb] Reject an unusable memory-cache-line-size

[qiyao]

target.process.memory-cache-line-size is an unbounded UInt64, so
settings set accepts 0, and MemoryCache keeps the value in a
uint32_t, so it also accepts any multiple of 2^32, which truncates to
0. Every consumer then takes a remainder by 0.

Process::ReadCStringFromMemory divides on the first iteration of its
loop, before it touches inferior memory, so any address reproduces it.
On an x86_64 host that is a SIGFPE and lldb dies; on AArch64 udiv by
zero yields 0, so addr % 0 evaluates to addr and the subtraction
underflows to a huge chunk size, and the bug hides.

$ lldb -b \
    -o 'settings set target.process.memory-cache-line-size 0' \
    -o 'target create --core linux-x86_64.core'
Floating point exception: 8

Under UBSan on any host, with
lldb/test/API/tools/lldb-dap/coreFile/linux-x86_64.core:

lldb/source/Target/Process.cpp:2388:40: runtime error: division by zero
    #0 lldb_private::Process::ReadCStringFromMemory
    #2 ProcessElfCore::GetMainExecutableModuleSpec
    #3 ProcessElfCore::DoLoadCore
    #4 lldb_private::Process::LoadCore

The truncating case crashes the same way while settings show reports a
value that is not 0, so nothing in the UI hints at the cause:

(lldb) settings set target.process.memory-cache-line-size 4294967296
(lldb) settings show target.process.memory-cache-line-size
target.process.memory-cache-line-size (unsigned) = 4294967296

The cache uses 0. 4294967297 truncates to 1, so the cache uses a
one-byte line.

Bound the property once instead of at each use, from 1 to UINT32_MAX,
the way Debugger bounds term-width and term-height. Rejecting
beats clamping, because settings show reads the stored value back and
would otherwise report a number the cache does not use:

(lldb) settings set target.process.memory-cache-line-size 4294967296
error: 4294967296 is out of range, valid values must be between 1 and 4294967295.

OptionValueProperties::CreateLocalCopy deep-copies the option values,
so the bounds also apply to each process's own collection, not just the
global one.

[llvmorg-github-actions]

@llvm/pr-subscribers-lldb

Author: Yao Qi (qiyao)

Changes

target.process.memory-cache-line-size is an unbounded UInt64, so
settings set accepts 0, and MemoryCache keeps the value in a
uint32_t, so it also accepts any multiple of 2^32, which truncates to
0. Every consumer then takes a remainder by 0.

Process::ReadCStringFromMemory divides on the first iteration of its
loop, before it touches inferior memory, so any address reproduces it.
On an x86_64 host that is a SIGFPE and lldb dies; on AArch64 udiv by
zero yields 0, so addr % 0 evaluates to addr and the subtraction
underflows to a huge chunk size, and the bug hides.

$ lldb -b \
    -o 'settings set target.process.memory-cache-line-size 0' \
    -o 'target create --core linux-x86_64.core'
Floating point exception: 8

Under UBSan on any host, with
lldb/test/API/tools/lldb-dap/coreFile/linux-x86_64.core:

lldb/source/Target/Process.cpp:2388:40: runtime error: division by zero
    #<!-- -->0 lldb_private::Process::ReadCStringFromMemory
    #<!-- -->2 ProcessElfCore::GetMainExecutableModuleSpec
    #<!-- -->3 ProcessElfCore::DoLoadCore
    #<!-- -->4 lldb_private::Process::LoadCore

The truncating case crashes the same way while settings show reports a
value that is not 0, so nothing in the UI hints at the cause:

(lldb) settings set target.process.memory-cache-line-size 4294967296
(lldb) settings show target.process.memory-cache-line-size
target.process.memory-cache-line-size (unsigned) = 4294967296

The cache uses 0. 4294967297 truncates to 1, so the cache uses a
one-byte line.

Bound the property once instead of at each use, from 1 to UINT32_MAX,
the way Debugger bounds term-width and term-height. Rejecting
beats clamping, because settings show reads the stored value back and
would otherwise report a number the cache does not use:

(lldb) settings set target.process.memory-cache-line-size 4294967296
error: 4294967296 is out of range, valid values must be between 1 and 4294967295.

OptionValueProperties::CreateLocalCopy deep-copies the option values,
so the bounds also apply to each process's own collection, not just the
global one.

---

Full diff: https://github.com/llvm/llvm-project/pull/219217.diff

2 Files Affected:

• (modified) lldb/source/Target/Process.cpp (+8)
• (modified) lldb/unittests/Target/MemoryTest.cpp (+39)

diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index fdc56e1c310eb..033638f680934 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -38,6 +38,7 @@
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/OptionArgParser.h"
 #include "lldb/Interpreter/OptionValueProperties.h"
+#include "lldb/Interpreter/OptionValueUInt64.h"
 #include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Target/ABI.h"
@@ -173,6 +174,13 @@ ProcessProperties::ProcessProperties(lldb_private::Process *process)
     // Global process properties, set them up one time
     m_collection_sp = std::make_shared<ProcessOptionValueProperties>("process");
     m_collection_sp->Initialize(g_process_properties_def);
+    // MemoryCache divides by the cache line size and holds it in a uint32_t, so
+    // reject a value it could not use.
+    OptionValueUInt64 *line_size =
+        m_collection_sp->GetPropertyAtIndexAsOptionValueUInt64(
+            ePropertyMemCacheLineSize);
+    line_size->SetMinimumValue(1);
+    line_size->SetMaximumValue(UINT32_MAX);
     m_collection_sp->AppendProperty(
         "thread", "Settings specific to threads.", true,
         Thread::GetGlobalProperties().GetValueProperties());
diff --git a/lldb/unittests/Target/MemoryTest.cpp b/lldb/unittests/Target/MemoryTest.cpp
index 73f17ca4ce122..97402c4cb6e03 100644
--- a/lldb/unittests/Target/MemoryTest.cpp
+++ b/lldb/unittests/Target/MemoryTest.cpp
@@ -445,6 +445,45 @@ TEST_F(MemoryTest, TestReadStopsAtAnInvalidRange) {
   EXPECT_TRUE(inside_error.Fail());
 }
 
+TEST_F(MemoryTest, TestUnusableCacheLineSize) {
+  ArchSpec arch("arm64-apple-macosx");
+
+  Platform::SetHostPlatform(PlatformRemoteMacOSX::CreateInstance(true, &arch));
+
+  DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+
+  // A Process copies the global properties when it is constructed, so the
+  // setting must be in place before CreateProcess, and put back afterwards.
+  struct SettingGuard {
+    ~SettingGuard() {
+      Process::GetGlobalProperties().SetPropertyValue(
+          nullptr, eVarSetOperationClear, "memory-cache-line-size", "");
+    }
+  } restore_setting;
+
+  auto set_line_size = [](const char *setting) {
+    return Process::GetGlobalProperties().SetPropertyValue(
+        nullptr, eVarSetOperationAssign, "memory-cache-line-size", setting);
+  };
+
+  // A usable setting must take effect, or the checks below prove nothing.
+  ASSERT_TRUE(set_line_size("256").Success());
+  TargetSP target_sp = CreateTarget(debugger_sp, arch);
+  DummyProcess *process =
+      static_cast<DummyProcess *>(CreateProcess(target_sp).get());
+  EXPECT_EQ(process->GetMemoryCacheLineSize(), 256u);
+
+  for (const char *setting : {"0", "4294967296"}) {
+    SCOPED_TRACE(setting);
+    EXPECT_TRUE(set_line_size(setting).Fail());
+    // Refused, so the last usable value is still in effect.
+    EXPECT_EQ(process->GetMemoryCacheLineSize(), 256u);
+    TargetSP later_target_sp = CreateTarget(debugger_sp, arch);
+    EXPECT_EQ(CreateProcess(later_target_sp)->GetMemoryCacheLineSize(), 256u);
+  }
+}
+
 TEST_F(MemoryTest, TestReadInteger) {
   ArchSpec arch("x86_64-apple-macosx-");
 

[jasonmolenda]

This looks good to me. I might have treated a cache line size of 0 as meaning "disable the memory read cache", duplicating the behavior of target.process.disable-memory-cache. But this is probably a better approach.

[qiyao]

This looks good to me. I might have treated a cache line size of 0 as meaning "disable the memory read cache", duplicating the behavior of target.process.disable-memory-cache. But this is probably a better approach.

My impression was similar, set cache line size of 0 means "disable the L2 cache". However, Process::ReadCStringFromMemory already assumes cache line size can't be zero.

llvm-project/lldb/source/Target/Process.cpp

Lines 2379 to 2380 in 0ab4db4

	addr_t cache_line_bytes_left =
	cache_line_size - (curr_addr % cache_line_size);