Skip to content
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

Complete rewrite of pcustom command #794

Merged
merged 17 commits into from
Jan 25, 2022
Merged

Complete rewrite of pcustom command #794

merged 17 commits into from
Jan 25, 2022

Conversation

hugsy
Copy link
Owner

@hugsy hugsy commented Jan 18, 2022

Description/Motivation/Screenshots

This PR is a complete rewrite of the pcustom command (and all its subcommands). It creates and uses a new ExternalStructureLoader class that efficiently allows to manipulate custom ctypes.Structures located in gef.config["pcustom.struct_path"].

On top of being way more Pythonic, the great advantage of this approach is that now any command/function can use those new imported types, which is much more preferable to the old approach.

This is a first step towards what was described in #778

How Has This Been Tested?

Architecture Yes/No Comments
x86-32 ✔️
x86-64 ✔️
ARM ✔️
AARCH64 ✔️
MIPS ✖️
POWERPC ✖️
SPARC ✖️
RISC-V ✖️
make test ✔️

Checklist

  • My PR was done against the dev branch, not master.
  • My code follows the code style of this project.
  • My change includes a change to the documentation, if required.
  • My change adds tests as appropriate.
  • I have read and agree to the CONTRIBUTING document.

Copy link
Collaborator

@Grazfather Grazfather left a comment

Choose a reason for hiding this comment

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

Done first pass.

gef.py Outdated Show resolved Hide resolved
gef.py Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
 - added cache
gef.py Outdated Show resolved Hide resolved
gef.py Outdated
@@ -5467,8 +5475,9 @@ def do_invoke(self, argv: List) -> None:
info(f"Listing custom structures from '{manager.path}'")
struct_color = gef.config["pcustom.structure_type"]
filename_color = gef.config["pcustom.structure_name"]
for module in manager.modules:
__modules = ", ".join([Color.colorify(structure.name, struct_color) for structure in module.structures])
for module_name in manager.modules:
Copy link
Collaborator

Choose a reason for hiding this comment

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

for module in manager.modules.values():

Copy link
Owner Author

@hugsy hugsy Jan 19, 2022

Choose a reason for hiding this comment

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

not a dict now changed to a subclass of dict

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is now?

gef.py Outdated Show resolved Hide resolved
gef.py Outdated
_class = getattr(module, self.name)
return _class

def apply_at(self, address: int, depth: int = 0, max_depth: int = -1) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

But I mean that the check below: if depth >= max_depth: will never pass with the default max_depth, unless depth is negative, which doesn't make sense.

If you want it to be a mandatory arg, you can make it a normal arg, or if you want it a mandatory kwarg use the snippet I posted above.

gef.py Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated
Comment on lines 5396 to 5397
for structure_name in module.structures:
yield module, module.structures[structure_name]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for structure_name in module.structures:
yield module, module.structures[structure_name]
for structure in module.structures.values():
yield module, structure

Copy link
Collaborator

Choose a reason for hiding this comment

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

This one should work

gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Show resolved Hide resolved
Copy link
Collaborator

@theguy147 theguy147 left a comment

Choose a reason for hiding this comment

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

The code logic looks good. Just a few remarks about the function signatures...

Also from typing import Set is no longer used as a consequence of this PR and should therefore be removed.

gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@daniellimws daniellimws left a comment

Choose a reason for hiding this comment

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

In the old version of pcustom, this error will show when the struct doesn't exist

gef➤  dt blah
[!] Invalid structure name 'blah'

Now it doesn't give any error when ran with a struct that doesn't exist.

gef➤  dt blah
[*] 'get_gef_setting' is deprecated and will be removed in a feature release. Use `gef.config[key]`
gef➤

gdb.execute("pcustom list")
return

modname, structname = self.get_modulename_structname_from_arg(argv[0])
_, structname = self.explode_type(args.type)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently we don't provide any error message when the struct does not exist. It will be helpful to have one that tells the user how to define a struct.

Suggested change
_, structname = self.explode_type(args.type)
_, structname = self.explode_type(args.type)
manager = ExternalStructureManager()
struct_info = manager.find(structname)
if not struct_info:
path = pathlib.Path(gef.config["pcustom.struct_path"]).expanduser()
struct_color = gef.config["pcustom.structure_type"]
filename_color = gef.config["pcustom.structure_name"]
err(f"No struct {Color.colorify(structname, struct_color)} found. "
f"Use {Color.yellowify('pcustom edit')} or create a file in {Color.colorify(path, filename_color)} "
"to define its fields.")
return

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually no, this should be done in ExternalStructureManger.find so that it applies for the other pcustom subcommands as well. Ignore this suggestion.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think you were right the first time: the caller should handle the error (a-la str.find() )

Copy link
Owner Author

Choose a reason for hiding this comment

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

Fixed as well, it is the caller's job to check the return value for ESM.find($structure_name) so now we have:

gef➤  pcustom
[+] Listing custom structures from '/tmp/gef/structs'
 →  /tmp/gef/structs/goo.py (foo_t, goo_t)
gef➤  pcustom meh
[!] No structure named 'goo' found

gef.py Outdated
Comment on lines 5449 to 5453
address = parse_address(args.address)
result = manager.find(structname)
if result:
_, structure = result
structure.apply_at(address, self["max_depth"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

manager.find(structname) moved up as per suggestion above

Suggested change
address = parse_address(args.address)
result = manager.find(structname)
if result:
_, structure = result
structure.apply_at(address, self["max_depth"])
address = parse_address(args.address)
_, structure = struct_info
structure.apply_at(address, self["max_depth"])
return

Copy link
Collaborator

Choose a reason for hiding this comment

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

@hugsy You only added the check to PCustomShowCommand, and there's no else branch here. So when running dt <invalid struct> <address>, there's still no error message.

gef.py Show resolved Hide resolved
Copy link
Collaborator

@daniellimws daniellimws left a comment

Choose a reason for hiding this comment

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

Sorry about the separate reviews, this is the last one.

That's all from me.

gef.py Outdated Show resolved Hide resolved
Co-authored-by: theguy147 <37738506+theguy147@users.noreply.github.com>
gef.py Outdated Show resolved Hide resolved
- passing a reference to `ExternalStructureLoader` to modules and structures for nested structure to work properly
Co-authored-by: Grazfather <grazfather@gmail.com>
Copy link
Collaborator

@daniellimws daniellimws left a comment

Choose a reason for hiding this comment

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

All looks good. Just missing out writing the error message in PCustomCommand.

gef.py Outdated
Comment on lines 5449 to 5453
address = parse_address(args.address)
result = manager.find(structname)
if result:
_, structure = result
structure.apply_at(address, self["max_depth"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

@hugsy You only added the check to PCustomShowCommand, and there's no else branch here. So when running dt <invalid struct> <address>, there's still no error message.

@hugsy
Copy link
Owner Author

hugsy commented Jan 25, 2022

Another good catch, so I've also added some CI checks for those error messages.

@hugsy hugsy merged commit 779369f into dev Jan 25, 2022
@hugsy hugsy deleted the fix_pcustom_command branch January 25, 2022 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants