[Lldb-commits] [lldb] [lldb] Handle an empty SBMemoryRegionInfo from scripted process (PR #115963)
Jason Molenda via lldb-commits
lldb-commits at lists.llvm.org
Thu Nov 14 23:32:47 PST 2024
https://github.com/jasonmolenda updated https://github.com/llvm/llvm-project/pull/115963
>From f8f1d70d1d9eac6d36c0fa84e2a94c032385da39 Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Tue, 12 Nov 2024 15:55:15 -0800
Subject: [PATCH 1/6] [lldb] Handle an empty SBMemoryRegionInfo from scripted
process
A scripted process implementation might return an SBMemoryRegionInfo
object in its implementation of `get_memory_region_containing_address`
which will have an address 0 and size 0, without realizing the
problems this can cause. One problem happens when an expression
is run, and lldb needs to find part of the target's virtual address
space to store the result (actually stored in lldb's host memory),
it uses IRMemoryMap::FindSpace() to do that. If the process supports
GetMemoryRegionInfo, FindSpace will step through the allocated
memory ranges looking for an unused one. But if every request is
a memory region with address 0 + size 0, this loop will never
terminate. Detect this kind of response from a scripted process
plugin and return an error, so callers iterating over the address
space can exit.
Added a test where I copied dummy_scripted_process.py from the
TestScriptedProcess.py existing test, and added a bad
`get_memory_region_containing_address` implementation.
rdar://139678032
---
.../Process/scripted/ScriptedProcess.cpp | 9 +-
.../Makefile | 3 +
.../TestScriptedProcessEmptyMemoryRegion.py | 33 +++++
.../dummy_scripted_process.py | 137 ++++++++++++++++++
.../main.c | 1 +
5 files changed, 182 insertions(+), 1 deletion(-)
create mode 100644 lldb/test/API/functionalities/scripted_process_empty_memory_region/Makefile
create mode 100644 lldb/test/API/functionalities/scripted_process_empty_memory_region/TestScriptedProcessEmptyMemoryRegion.py
create mode 100644 lldb/test/API/functionalities/scripted_process_empty_memory_region/dummy_scripted_process.py
create mode 100644 lldb/test/API/functionalities/scripted_process_empty_memory_region/main.c
diff --git a/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp b/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
index d2111ce877ce55..c56e24a4ebd188 100644
--- a/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ b/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -288,8 +288,15 @@ Status ScriptedProcess::DoGetMemoryRegionInfo(lldb::addr_t load_addr,
MemoryRegionInfo ®ion) {
Status error;
if (auto region_or_err =
- GetInterface().GetMemoryRegionContainingAddress(load_addr, error))
+ GetInterface().GetMemoryRegionContainingAddress(load_addr, error)) {
region = *region_or_err;
+ if (region.GetRange().GetRangeBase() == 0 &&
+ (region.GetRange().GetByteSize() == 0 ||
+ region.GetRange().GetByteSize() == LLDB_INVALID_ADDRESS)) {
+ error = Status::FromErrorString(
+ "Invalid memory region from scripted process");
+ }
+ }
return error;
}
diff --git a/lldb/test/API/functionalities/scripted_process_empty_memory_region/Makefile b/lldb/test/API/functionalities/scripted_process_empty_memory_region/Makefile
new file mode 100644
index 00000000000000..10495940055b63
--- /dev/null
+++ b/lldb/test/API/functionalities/scripted_process_empty_memory_region/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
diff --git a/lldb/test/API/functionalities/scripted_process_empty_memory_region/TestScriptedProcessEmptyMemoryRegion.py b/lldb/test/API/functionalities/scripted_process_empty_memory_region/TestScriptedProcessEmptyMemoryRegion.py
new file mode 100644
index 00000000000000..1ff084cfb0278e
--- /dev/null
+++ b/lldb/test/API/functionalities/scripted_process_empty_memory_region/TestScriptedProcessEmptyMemoryRegion.py
@@ -0,0 +1,33 @@
+"""
+Test python scripted process which returns an empty SBMemoryRegionInfo
+"""
+
+import os, shutil
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test import lldbtest
+
+
+class ScriptedProcessEmptyMemoryRegion(TestBase):
+ NO_DEBUG_INFO_TESTCASE = True
+
+ def test_scripted_process_empty_memory_region(self):
+ """Test that lldb handles an empty SBMemoryRegionInfo object from
+ a scripted process plugin."""
+ self.build()
+
+ target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+ self.assertTrue(target, VALID_TARGET)
+
+ scripted_process_example_relpath = "dummy_scripted_process.py"
+ self.runCmd(
+ "command script import "
+ + os.path.join(self.getSourceDir(), scripted_process_example_relpath)
+ )
+
+ self.expect("memory region 0", error=True, substrs=["Invalid memory region"])
+
+ self.expect("expr -- 5", substrs=["5"])
diff --git a/lldb/test/API/functionalities/scripted_process_empty_memory_region/dummy_scripted_process.py b/lldb/test/API/functionalities/scripted_process_empty_memory_region/dummy_scripted_process.py
new file mode 100644
index 00000000000000..8ca828eef26f01
--- /dev/null
+++ b/lldb/test/API/functionalities/scripted_process_empty_memory_region/dummy_scripted_process.py
@@ -0,0 +1,137 @@
+import os, struct, signal
+
+from typing import Any, Dict
+
+import lldb
+from lldb.plugins.scripted_process import ScriptedProcess
+from lldb.plugins.scripted_process import ScriptedThread
+
+
+class DummyStopHook:
+ def __init__(self, target, args):
+ self.target = target
+ self.args = args
+
+ def handle_stop(self, exe_ctx, stream):
+ print("My DummyStopHook triggered. Printing args: \n%s" % self.args)
+ sp = exe_ctx.process.GetScriptedImplementation()
+ sp.handled_stop = True
+
+
+class DummyScriptedProcess(ScriptedProcess):
+ memory = None
+
+ def __init__(self, exe_ctx: lldb.SBExecutionContext, args: lldb.SBStructuredData):
+ super().__init__(exe_ctx, args)
+ self.threads[0] = DummyScriptedThread(self, None)
+ self.memory = {}
+ addr = 0x500000000
+ debugger = self.target.GetDebugger()
+ index = debugger.GetIndexOfTarget(self.target)
+ self.memory[addr] = "Hello, target " + str(index)
+ self.handled_stop = False
+
+ def read_memory_at_address(
+ self, addr: int, size: int, error: lldb.SBError
+ ) -> lldb.SBData:
+ data = lldb.SBData().CreateDataFromCString(
+ self.target.GetByteOrder(), self.target.GetCodeByteSize(), self.memory[addr]
+ )
+
+ return data
+
+ def get_memory_region_containing_address(
+ self, addr: int
+ ) -> lldb.SBMemoryRegionInfo:
+ return lldb.SBMemoryRegionInfo()
+
+ def write_memory_at_address(self, addr, data, error):
+ self.memory[addr] = data.GetString(error, 0)
+ return len(self.memory[addr]) + 1
+
+ def get_loaded_images(self):
+ return self.loaded_images
+
+ def get_process_id(self) -> int:
+ return 42
+
+ def should_stop(self) -> bool:
+ return True
+
+ def is_alive(self) -> bool:
+ return True
+
+ def get_scripted_thread_plugin(self):
+ return DummyScriptedThread.__module__ + "." + DummyScriptedThread.__name__
+
+ def my_super_secret_method(self):
+ if hasattr(self, "my_super_secret_member"):
+ return self.my_super_secret_member
+ else:
+ return None
+
+
+class DummyScriptedThread(ScriptedThread):
+ def __init__(self, process, args):
+ super().__init__(process, args)
+ self.frames.append({"pc": 0x0100001B00})
+
+ def get_thread_id(self) -> int:
+ return 0x19
+
+ def get_name(self) -> str:
+ return DummyScriptedThread.__name__ + ".thread-1"
+
+ def get_state(self) -> int:
+ return lldb.eStateStopped
+
+ def get_stop_reason(self) -> Dict[str, Any]:
+ return {"type": lldb.eStopReasonTrace, "data": {}}
+
+ def get_register_context(self) -> str:
+ return struct.pack(
+ "21Q",
+ 1,
+ 2,
+ 3,
+ 4,
+ 5,
+ 6,
+ 7,
+ 8,
+ 9,
+ 10,
+ 11,
+ 12,
+ 13,
+ 14,
+ 15,
+ 16,
+ 17,
+ 18,
+ 19,
+ 20,
+ 21,
+ )
+
+
+def __lldb_init_module(debugger, dict):
+ # This is used when loading the script in an interactive debug session to
+ # automatically, register the stop-hook and launch the scripted process.
+ if not "SKIP_SCRIPTED_PROCESS_LAUNCH" in os.environ:
+ debugger.HandleCommand(
+ "target stop-hook add -k first -v 1 -k second -v 2 -P %s.%s"
+ % (__name__, DummyStopHook.__name__)
+ )
+ debugger.HandleCommand(
+ "process launch -C %s.%s" % (__name__, DummyScriptedProcess.__name__)
+ )
+ else:
+ print(
+ "Name of the class that will manage the scripted process: '%s.%s'"
+ % (__name__, DummyScriptedProcess.__name__)
+ )
+ print(
+ "Name of the class that will manage the stop-hook: '%s.%s'"
+ % (__name__, DummyStopHook.__name__)
+ )
diff --git a/lldb/test/API/functionalities/scripted_process_empty_memory_region/main.c b/lldb/test/API/functionalities/scripted_process_empty_memory_region/main.c
new file mode 100644
index 00000000000000..237c8ce181774d
--- /dev/null
+++ b/lldb/test/API/functionalities/scripted_process_empty_memory_region/main.c
@@ -0,0 +1 @@
+int main() {}
>From 0e577079709053e2f0c4cb81e118d2c8c6ddd471 Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Tue, 12 Nov 2024 18:23:37 -0800
Subject: [PATCH 2/6] Remove the clearly unnecessary parts from
dummy_scripted_process.py. Add a comment in
ScriptedProcess::DoGetMemoryRegionInfo about what types of memory regions are
rejecting.
---
.../Process/scripted/ScriptedProcess.cpp | 4 ++-
.../dummy_scripted_process.py | 33 ++-----------------
2 files changed, 6 insertions(+), 31 deletions(-)
diff --git a/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp b/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
index c56e24a4ebd188..eca4affd9005ce 100644
--- a/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ b/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -290,9 +290,11 @@ Status ScriptedProcess::DoGetMemoryRegionInfo(lldb::addr_t load_addr,
if (auto region_or_err =
GetInterface().GetMemoryRegionContainingAddress(load_addr, error)) {
region = *region_or_err;
+ // Reject a region of {0,0} or {0,UINT64_MAX}, neither are
+ // meaningful responses.
if (region.GetRange().GetRangeBase() == 0 &&
(region.GetRange().GetByteSize() == 0 ||
- region.GetRange().GetByteSize() == LLDB_INVALID_ADDRESS)) {
+ region.GetRange().GetByteSize() == UINT64_MAX)) {
error = Status::FromErrorString(
"Invalid memory region from scripted process");
}
diff --git a/lldb/test/API/functionalities/scripted_process_empty_memory_region/dummy_scripted_process.py b/lldb/test/API/functionalities/scripted_process_empty_memory_region/dummy_scripted_process.py
index 8ca828eef26f01..31e28a57122f66 100644
--- a/lldb/test/API/functionalities/scripted_process_empty_memory_region/dummy_scripted_process.py
+++ b/lldb/test/API/functionalities/scripted_process_empty_memory_region/dummy_scripted_process.py
@@ -7,17 +7,6 @@
from lldb.plugins.scripted_process import ScriptedThread
-class DummyStopHook:
- def __init__(self, target, args):
- self.target = target
- self.args = args
-
- def handle_stop(self, exe_ctx, stream):
- print("My DummyStopHook triggered. Printing args: \n%s" % self.args)
- sp = exe_ctx.process.GetScriptedImplementation()
- sp.handled_stop = True
-
-
class DummyScriptedProcess(ScriptedProcess):
memory = None
@@ -116,22 +105,6 @@ def get_register_context(self) -> str:
def __lldb_init_module(debugger, dict):
- # This is used when loading the script in an interactive debug session to
- # automatically, register the stop-hook and launch the scripted process.
- if not "SKIP_SCRIPTED_PROCESS_LAUNCH" in os.environ:
- debugger.HandleCommand(
- "target stop-hook add -k first -v 1 -k second -v 2 -P %s.%s"
- % (__name__, DummyStopHook.__name__)
- )
- debugger.HandleCommand(
- "process launch -C %s.%s" % (__name__, DummyScriptedProcess.__name__)
- )
- else:
- print(
- "Name of the class that will manage the scripted process: '%s.%s'"
- % (__name__, DummyScriptedProcess.__name__)
- )
- print(
- "Name of the class that will manage the stop-hook: '%s.%s'"
- % (__name__, DummyStopHook.__name__)
- )
+ debugger.HandleCommand(
+ "process launch -C %s.%s" % (__name__, DummyScriptedProcess.__name__)
+ )
>From 403fe3d304b0a2e17e721af5c20267df2e6f5c7c Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Tue, 12 Nov 2024 23:29:57 -0800
Subject: [PATCH 3/6] Update patch to move the check for memory regions to
Process::GetMemoryRegionInfo - I want to reject these regions from either gdb
remote serial protocol stubs, or scripted processes.
---
.../Plugins/Process/scripted/ScriptedProcess.cpp | 11 +----------
lldb/source/Target/Process.cpp | 11 ++++++++++-
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp b/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
index eca4affd9005ce..d2111ce877ce55 100644
--- a/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ b/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -288,17 +288,8 @@ Status ScriptedProcess::DoGetMemoryRegionInfo(lldb::addr_t load_addr,
MemoryRegionInfo ®ion) {
Status error;
if (auto region_or_err =
- GetInterface().GetMemoryRegionContainingAddress(load_addr, error)) {
+ GetInterface().GetMemoryRegionContainingAddress(load_addr, error))
region = *region_or_err;
- // Reject a region of {0,0} or {0,UINT64_MAX}, neither are
- // meaningful responses.
- if (region.GetRange().GetRangeBase() == 0 &&
- (region.GetRange().GetByteSize() == 0 ||
- region.GetRange().GetByteSize() == UINT64_MAX)) {
- error = Status::FromErrorString(
- "Invalid memory region from scripted process");
- }
- }
return error;
}
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index b99692b8a0bfd9..d792f86254e7e1 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -6184,7 +6184,16 @@ Status Process::GetMemoryRegionInfo(lldb::addr_t load_addr,
MemoryRegionInfo &range_info) {
if (const lldb::ABISP &abi = GetABI())
load_addr = abi->FixAnyAddress(load_addr);
- return DoGetMemoryRegionInfo(load_addr, range_info);
+ Status error = DoGetMemoryRegionInfo(load_addr, range_info);
+ // Reject a region of {0,0} or {0,UINT64_MAX}, neither are
+ // meaningful responses.
+ if (error.Success() && range_info.GetRange().GetRangeBase() == 0 &&
+ (range_info.GetRange().GetByteSize() == 0 ||
+ range_info.GetRange().GetByteSize() == UINT64_MAX))
+ error =
+ Status::FromErrorString("Invalid memory region from scripted process");
+
+ return error;
}
Status Process::GetMemoryRegions(lldb_private::MemoryRegionInfos ®ion_list) {
>From aadf21dddfd0f053956b93cd431272e3f237d646 Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Wed, 13 Nov 2024 10:28:05 -0800
Subject: [PATCH 4/6] Update error message.
---
lldb/source/Target/Process.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index d792f86254e7e1..ba4701e2660847 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -6191,7 +6191,7 @@ Status Process::GetMemoryRegionInfo(lldb::addr_t load_addr,
(range_info.GetRange().GetByteSize() == 0 ||
range_info.GetRange().GetByteSize() == UINT64_MAX))
error =
- Status::FromErrorString("Invalid memory region from scripted process");
+ Status::FromErrorString("Invalid memory region");
return error;
}
>From d4125c14282910fc240dfa9a0d93b8b68e113ba2 Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Thu, 14 Nov 2024 18:56:51 -0800
Subject: [PATCH 5/6] Change the test to be "returned region must contain addr"
This will reject a 0-length memory region response, e.g.
an empty SBMemoryRegionInfo object.
---
lldb/source/Target/Process.cpp | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index ba4701e2660847..9c53256dd5dee5 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -6185,11 +6185,9 @@ Status Process::GetMemoryRegionInfo(lldb::addr_t load_addr,
if (const lldb::ABISP &abi = GetABI())
load_addr = abi->FixAnyAddress(load_addr);
Status error = DoGetMemoryRegionInfo(load_addr, range_info);
- // Reject a region of {0,0} or {0,UINT64_MAX}, neither are
- // meaningful responses.
- if (error.Success() && range_info.GetRange().GetRangeBase() == 0 &&
- (range_info.GetRange().GetByteSize() == 0 ||
- range_info.GetRange().GetByteSize() == UINT64_MAX))
+ // Reject a region that does not contain the requested address.
+ if (error.Success() && (range_info.GetRange().GetRangeBase() < load_addr ||
+ range_info.GetRange().GetRangeEnd() <= load_addr))
error =
Status::FromErrorString("Invalid memory region");
>From f79aa7362f31286eddc60aec7d6a1f53a8682f6f Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Thu, 14 Nov 2024 23:32:12 -0800
Subject: [PATCH 6/6] Ismail caught a logic error on my part, used a built-in
method instead.
---
lldb/source/Target/Process.cpp | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 9c53256dd5dee5..9125ceca74a003 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -6186,10 +6186,8 @@ Status Process::GetMemoryRegionInfo(lldb::addr_t load_addr,
load_addr = abi->FixAnyAddress(load_addr);
Status error = DoGetMemoryRegionInfo(load_addr, range_info);
// Reject a region that does not contain the requested address.
- if (error.Success() && (range_info.GetRange().GetRangeBase() < load_addr ||
- range_info.GetRange().GetRangeEnd() <= load_addr))
- error =
- Status::FromErrorString("Invalid memory region");
+ if (error.Success() && !range_info.GetRange().Contains(load_addr))
+ error = Status::FromErrorString("Invalid memory region");
return error;
}
More information about the lldb-commits
mailing list