-
-
Notifications
You must be signed in to change notification settings - Fork 732
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use info proc mapping
#1046
Use info proc mapping
#1046
Conversation
Putting on hold for now because gdb on 20.04 uses a different format for
as opposed to 22.04
i.e. missing perms for 20.04 Maybe go ahead with the change to ubuntu 24.04? |
Maybe we can check for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just some nitpicks.
tests/api/gef_memory.py
Outdated
size = int(parts[2], 16) | ||
int(parts[3], 16) | ||
assert end_addr == start_addr + size | ||
assert len(parts[4]) == 4, parts[4] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's with this second part of the assertion? That's the error message. We could probably give a better explanation (or drop it altogether)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad debug print, fixed
tests/base.py
Outdated
@property | ||
def gdb_version(self) -> Tuple[int, int]: | ||
res = tuple( | ||
map(int, re.search(r"(\d+)[^\d]+(\d+)", self._gdb.VERSION).groups()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this for a version with just 2 numbers? We might want something like r"(\d+)\D(\d+)"
. I don't think we need number +
non-digits, and \D
is the same as [^\d]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH the intent is also to let things blow here because this is the same pattern we use and trust in gef. Having it fail here could hint issue with that in gef itself.
res = tuple( | ||
map(int, re.search(r"(\d+)[^\d]+(\d+)", self._gdb.VERSION).groups()) | ||
) | ||
assert len(res) >= 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The groups
call will fail if there aren't exactly two matches. re.search
will return None
if the version string doesn't match against the regex, and then groups()
will raise an AttributeError
.
You could instead assert that .search
returns a match, and then check the length of groups
(it will always be 2), or just drop this stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should drop this. The search.groups()
will already raise the exception. This assert is impossible to fail.
tests/base.py
Outdated
res = tuple( | ||
map(int, re.search(r"(\d+)[^\d]+(\d+)", self._gdb.VERSION).groups()) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
res = tuple( | |
map(int, re.search(r"(\d+)[^\d]+(\d+)", self._gdb.VERSION).groups()) | |
) | |
res = [int(d) for d in re.search(r"(\d+)\D(\d+)", self._gdb.VERSION).groups()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure ok
Can we split out the CI / coverage changes into its own PR? |
Done, see #1050 |
## Description Restore the coverage comment functionality for PRs to a workable state The reason it's broken is because of different security permissions between actions triggered by `pull_request` and `pull_request_target`. This will need to be improved carefully by testing non-project commitors successfully trigger the action.
Description
Use
info proc mapping
as a first memory layout enumeration technique.Removed
maintenance info sections
which is not about memory layoutChecklist