Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) 

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@labath labath Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}.

Copy link
Collaborator

@labath labath Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@jasonmolenda jasonmolenda Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

error = Status::FromErrorString(
"Invalid memory region from scripted process");
}
}

return error;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
C_SOURCES := main.c

include Makefile.rules
Original file line number Diff line number Diff line change
@@ -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"])
Original file line number Diff line number Diff line change
@@ -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


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need this

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__)
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or this

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
int main() {}
Loading