-
Notifications
You must be signed in to change notification settings - Fork 633
ghidra: switch to PyGhidra #2788
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
base: master
Are you sure you want to change the base?
Conversation
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.
Please add bug fixes, new features, breaking changes and anything else you think is worthwhile mentioning to the master (unreleased) section of CHANGELOG.md. If no CHANGELOG update is needed add the following to the PR description: [x] No CHANGELOG update needed
Summary of ChangesHello @mike-hunhoff, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the Ghidra integration for capa by switching from the Ghidrathon extension to PyGhidra, Ghidra's official Python 3 binding. This change aims to provide a more robust and streamlined analysis experience, centralizing Ghidra API interactions through a new context management system and simplifying the user workflow by integrating directly into the Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request is a significant and well-executed refactoring to switch the Ghidra backend to PyGhidra. This change greatly improves the user experience by allowing capa to be run directly from the command line with Ghidra as a backend, rather than requiring scripts to be executed within the Ghidra environment. The changes are comprehensive, covering application logic, testing, and documentation.
I've identified a few areas for minor improvements related to code style and robustness, such as moving local imports to the module level for better clarity, restoring type hints for maintainability, and using more specific exception handling. Overall, this is an excellent contribution.
| def __init__(self, ctx_manager=None, tmpdir=None): | ||
| self.ctx_manager = ctx_manager | ||
| self.tmpdir = tmpdir | ||
| import capa.features.extractors.ghidra.helpers as ghidra_helpers |
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 import of ghidra_helpers is local to the __init__ method, which requires other methods in this class to re-import it. To improve code clarity and adhere to standard Python style (PEP 8), this import should be moved to the top of the file.
After moving this import, please remove the redundant local imports in get_base_address (line 62) and get_function (line 93).
| return ints_to_bytes(getBytes(addr, length)) # type: ignore [name-defined] # noqa: F821 | ||
| except RuntimeError: | ||
| return ints_to_bytes(get_flat_api().getBytes(addr, int(length))) | ||
| except Exception: |
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.
Catching a broad Exception can hide unexpected errors. The original RuntimeError was likely more specific to exceptions translated from the Java layer. It's better to be as specific as possible with exception handling. Please consider reverting to RuntimeError or catching a more specific set of exceptions if other types are expected.
| except Exception: | |
| except RuntimeError: |
CHANGELOG updated or no update needed, thanks! 😄
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
mr-tz
left a comment
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.
With tests passing (after fixing or xfailing them for now) this looks solid! May want to consult @colton-gabertan as well.
Vibe coding a switch to PyGhidra for the Ghidra backend. PyGhidra is Ghidra's native Python 3 support, so we should use it.