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

Fix capstone-disassemble command and documentation #695

Merged
merged 13 commits into from
Aug 23, 2021
Merged

Conversation

theguy147
Copy link
Collaborator

Fix capstone-disassemble command and documentation

Description/Motivation/Screenshots

As part of #693 this PR fixes the documentation and help messages for the capstone-disassemble command. Furthermore the command was not working in use cases like emulating aarch64 inside qemu. This was due to an uncaught exception caused in gef_get_auxiliary_values when gdb's info auxv doesn't return useful results. This bug has been fixed. Also resolving of LOCATIONs has been fixed.

With this PR I also propose changing the syntax for the location argument back to how it was before argparse: as a positional argument.

During this PR I introduced a new convenience wrapper function around gdb's parse_and_eval to specifically resolve LOCATIONs. This way other commands can rely on it without having to handle the different possible cases neccessary (because e.g. gdb resolves registers different from symbols and for symbols a simple conversion to int does not work directly). This new function does not aim at replacing gef_safe_parse_and_eval at this point because too many commands rely currently rely on it and there might be use cases that don't resolve locations.

I also added a unit test that checks if resolving a symbol (e.g. "main") works for the command.

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.

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
gef.py Outdated Show resolved Hide resolved
gef.py Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
Copy link
Owner

@hugsy hugsy left a comment

Choose a reason for hiding this comment

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

Good PR, some stuff to change and we'll merge after. Thanks for updating the doc too.

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
@theguy147
Copy link
Collaborator Author

I think all requested changes have been implemented now.

Copy link
Owner

@hugsy hugsy left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM. Just a few comment nitpicks.

gef.py Outdated Show resolved Hide resolved
@Grazfather Grazfather merged commit dcfa6f2 into hugsy:dev Aug 23, 2021
@theguy147 theguy147 deleted the cs branch August 23, 2021 15:30
@hugsy hugsy added this to the Release: next milestone Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants