[lldb] Handle an empty SBMemoryRegionInfo from scripted process #115963

jasonmolenda · 2024-11-13T00:01:57Z

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. Several algorithms in lldb will try to iterate over the MemoryRegions of the process, starting at address 0 and expecting to iterate up to the highest vm address, stepping by the size of each region, so a 0-length region will result in an infinite loop. Add a check to Process::GetMemoryRegionInfo that rejects a MemoryRegion which does not contain the requested address; a 0-length memory region will therefor always be rejected.

rdar://139678032

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

llvmbot · 2024-11-13T00:02:36Z

@llvm/pr-subscribers-lldb

Author: Jason Molenda (jasonmolenda)

Changes

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

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

5 Files Affected:

(modified) lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp (+8-1)
(added) lldb/test/API/functionalities/scripted_process_empty_memory_region/Makefile (+3)
(added) lldb/test/API/functionalities/scripted_process_empty_memory_region/TestScriptedProcessEmptyMemoryRegion.py (+33)
(added) lldb/test/API/functionalities/scripted_process_empty_memory_region/dummy_scripted_process.py (+137)
(added) lldb/test/API/functionalities/scripted_process_empty_memory_region/main.c (+1)

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 &region) {
   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() {}

jasonmolenda · 2024-11-13T00:04:14Z

I am uncertain where I should detect an improperly formatted memory range. It could be in Process::GetMemoryRegionInfo, which would catch a bad gdb remote serial protocol response as well. Or it could be localized to IRMemoryMap::FindSpace if we only wanted to avoid an infinite loop in that one place -- but there are other parts of the code that try to iterate over the address space in the target with these objects. This patch adjusts it in the most specific place that fixed the issue I was looking at, to start with.

medismailben

LGTM with comments.

medismailben · 2024-11-13T00:11:55Z

lldb/test/API/functionalities/scripted_process_empty_memory_region/dummy_scripted_process.py

+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
+
+


You shouldn't need this

medismailben · 2024-11-13T00:12:12Z

lldb/test/API/functionalities/scripted_process_empty_memory_region/dummy_scripted_process.py

+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__)
+        )


JDevlieghere · 2024-11-13T00:29:56Z

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp

    region = *region_or_err;
+    if (region.GetRange().GetRangeBase() == 0 &&
+        (region.GetRange().GetByteSize() == 0 ||
+         region.GetRange().GetByteSize() == LLDB_INVALID_ADDRESS)) {


Why would the size be LLDB_INVALID_ADDRESS? Did you mean to check that the range base isn't LLDB_INVALID_ADDRESS?

Is this the condition you want?

(region.GetRange().GetRangeBase() == 0 || region.GetRange().GetRangeBase() == LLDB_INVALID_ADDRESS) && region.GetRange().GetByteSize() == 0)

Last week I had another similar bug where a gdb remote stub was returning start addr 0 size UINT64_MAX as a region response, and I was trying to reject that in addition to addr 0 size 0. I haven't seen someone send that same incorrect response from a scripted process, though, I should just check for 0/0 here.

I'm not wedded to it. I'm still debating moving both of these checks up in to Process::GetMemoryRegionInfo. I haven't done it yet for fear of unintended consequences, but any memory region info response that claims 0 bytes or the entire address space is meaningless.

A single region that covers the entire address space is fairly boring, but it is the kind of thing I might return as an "I don't know" value (perhaps I'm in some embedded environment where every address really is readable?). What's the problem with that kind of result?

Also, I think this check could be made more robust. (0, 0) is the likeliest response, but doesn't this mean that we would still loop if the plugin returns (0,1) instead? It sounds to me that if we check that:

the returned region is of non-zero size

the returned region actually includes the address being queried (I guess this kinda subsumes the previous item)

then we can guarantee forward progress no matter what the plugin returns (?)

We had a gdb stub returning {0, UINT64_MAX} the other week and it broke IRMemory::FindSpace() which will avoid any memory region with read/write/execute flags if qMemoryRegionInfo packets are supported. The stub claimed the entire address space, FindSpace() said it could not find any address range available, and all expressions broke.

Yeah, a range of {0, 1} would result in algorithms like FindSpace() looping for a very long time, and be nearly as bad. But so far the two instances I've seen of people return bad ranges are {0,0} and {0,UINT64_MAX}.

We had a gdb stub returning {0, UINT64_MAX} the other week and it broke IRMemory::FindSpace() which will avoid any memory region with read/write/execute flags if qMemoryRegionInfo packets are supported. The stub claimed the entire address space, FindSpace() said it could not find any address range available, and all expressions broke.

Okay, but what's the alternative? Picking a piece of memory that may overlap with some existing data? It sounds to me like the stub gets exactly what it asked for.

Yeah, a range of {0, 1} would result in algorithms like FindSpace() looping for a very long time, and be nearly as bad. But so far the two instances I've seen of people return bad ranges are {0,0} and {0,UINT64_MAX}.

True, but if we can change the expression to catch both, why not do it? What I'm suggesting is to change the expression into something like if (GetRange().GetRangeBase() > addr || GetRange().GetRangeEnd() <= addr). The (0,0) case is subsumed by that, but this also catches any other incorrect response.

I see what you mean for checking the address is contained within the range, and rejecting it if not, this works too.

Right now if a stub returns {0, UINT64_MAX} and we don't have a memory allocation packet, IRMemory::FindSpace will find no available virtual address space and all expressions will fail.

My opposition to {0, UINT64_MAX} being accepted is that it's meaningless. No processor actually has 64-bits of virtually addressable memory, so telling me that all memory is addressable is obviously wrong. This response tells us nothing more than if qMemoryRegionInfo was an unsupported packet.

I guess the reason I don't like that is that it just feels like a very special case. Like if I send two regions, each covering half the address space, then I'm sort of saying the same thing, but unlike the single region, this response will get accepted. It also doesn't handle the case of a 32 bit address space being full. The fact that this breaks all of expression evaluation might be sort of a good thing actually, as it means the problem is likely to be caught.

If you feel strongly about this, then I don't want to stand in your way, but I would rather not have this special case.

No, your points make sense, thanks for sticking with this discussion. I'll update the patch as you suggest.

dummy_scripted_process.py. Add a comment in ScriptedProcess::DoGetMemoryRegionInfo about what types of memory regions are rejecting.

Process::GetMemoryRegionInfo - I want to reject these regions from either gdb remote serial protocol stubs, or scripted processes.

github-actions · 2024-11-13T18:32:03Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff b0a4e958e85784cff46303c92b6a3a14b20fa1d8 d4125c14282910fc240dfa9a0d93b8b68e113ba2 --extensions c,cpp -- lldb/test/API/functionalities/scripted_process_empty_memory_region/main.c lldb/source/Target/Process.cpp

View the diff from clang-format here.

diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 9c53256dd5..0afca3e4c3 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -6188,8 +6188,7 @@ Status Process::GetMemoryRegionInfo(lldb::addr_t load_addr,
   // 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");
+    error = Status::FromErrorString("Invalid memory region");
 
   return error;
 }

This will reject a 0-length memory region response, e.g. an empty SBMemoryRegionInfo object.

jasonmolenda · 2024-11-15T03:02:15Z

I landed a fix with Pavel's suggestion, and rewrote the commit msg to reflect what the patch now does.

JDevlieghere

LGTM with the formatting fixed.

medismailben

@jasonmolenda I think there is a mistake in your range check.

medismailben · 2024-11-15T06:09:51Z

lldb/source/Target/Process.cpp

+  if (error.Success() && (range_info.GetRange().GetRangeBase() < load_addr ||
+                          range_info.GetRange().GetRangeEnd() <= load_addr))


I believe in range_info.GetRange().GetRangeBase() < load_addr the sign should be inverted.

You could use directly the Range::{Contains, ContainsEndInclusive} instead ?

Suggested change

if (error.Success() && (range_info.GetRange().GetRangeBase() < load_addr ||

range_info.GetRange().GetRangeEnd() <= load_addr))

if (error.Success() && !range_info.GetRange().ContainsEndInclusive(load_addr))

jasonmolenda requested review from adrian-prantl and medismailben November 13, 2024 00:01

jasonmolenda requested a review from JDevlieghere as a code owner November 13, 2024 00:01

llvmbot added the lldb label Nov 13, 2024

medismailben approved these changes Nov 13, 2024

View reviewed changes

JDevlieghere reviewed Nov 13, 2024

View reviewed changes

jasonmolenda added 3 commits November 12, 2024 18:23

Remove the clearly unnecessary parts from

0e57707

dummy_scripted_process.py. Add a comment in ScriptedProcess::DoGetMemoryRegionInfo about what types of memory regions are rejecting.

Update patch to move the check for memory regions to

403fe3d

Process::GetMemoryRegionInfo - I want to reject these regions from either gdb remote serial protocol stubs, or scripted processes.

Update error message.

aadf21d

Change the test to be "returned region must contain addr"

d4125c1

This will reject a 0-length memory region response, e.g. an empty SBMemoryRegionInfo object.

JDevlieghere approved these changes Nov 15, 2024

View reviewed changes

medismailben requested changes Nov 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lldb] Handle an empty SBMemoryRegionInfo from scripted process #115963

[lldb] Handle an empty SBMemoryRegionInfo from scripted process #115963

jasonmolenda commented Nov 13, 2024 •

edited

Loading

llvmbot commented Nov 13, 2024

jasonmolenda commented Nov 13, 2024

medismailben left a comment

medismailben Nov 13, 2024

medismailben Nov 13, 2024

JDevlieghere Nov 13, 2024

jasonmolenda Nov 13, 2024

jasonmolenda Nov 13, 2024

labath Nov 13, 2024 •

edited

Loading

jasonmolenda Nov 13, 2024

labath Nov 14, 2024 •

edited

Loading

jasonmolenda Nov 14, 2024 •

edited

Loading

labath Nov 14, 2024

jasonmolenda Nov 15, 2024

github-actions bot commented Nov 13, 2024 •

edited

Loading

jasonmolenda commented Nov 15, 2024

JDevlieghere left a comment

medismailben left a comment

medismailben Nov 15, 2024

		if (error.Success() && (range_info.GetRange().GetRangeBase() < load_addr \|\|
		range_info.GetRange().GetRangeEnd() <= load_addr))

	if (error.Success() && (range_info.GetRange().GetRangeBase() < load_addr \|\|
	range_info.GetRange().GetRangeEnd() <= load_addr))
	if (error.Success() && !range_info.GetRange().ContainsEndInclusive(load_addr))

[lldb] Handle an empty SBMemoryRegionInfo from scripted process #115963

Are you sure you want to change the base?

[lldb] Handle an empty SBMemoryRegionInfo from scripted process #115963

Conversation

jasonmolenda commented Nov 13, 2024 • edited Loading

llvmbot commented Nov 13, 2024

jasonmolenda commented Nov 13, 2024

medismailben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

labath Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

labath Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

jasonmolenda Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 13, 2024 • edited Loading

jasonmolenda commented Nov 15, 2024

JDevlieghere left a comment

Choose a reason for hiding this comment

medismailben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmolenda commented Nov 13, 2024 •

edited

Loading

labath Nov 13, 2024 •

edited

Loading

labath Nov 14, 2024 •

edited

Loading

jasonmolenda Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 13, 2024 •

edited

Loading