-
Notifications
You must be signed in to change notification settings - Fork 639
loader: gracefully handle ELF files with unsupported architectures #2800
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
loader: gracefully handle ELF files with unsupported architectures #2800
Conversation
When analyzing ELF files with unsupported architectures (e.g., ARM64 variant), vivisect raises a generic Exception with message 'Unsupported Architecture: %d'. This was not caught by existing error handlers, causing capa to crash with an unfriendly error message. This change adds exception handling to detect the 'Unsupported Architecture' error message and convert it to a user-friendly CorruptFile exception, following the same pattern as the existing 'Couldn't convert rva' handler. The architecture number is extracted from the exception args and included in the error message to help users understand what went wrong. closes mandiant#2793
Summary of ChangesHello @kami922, 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 enhances the robustness of Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 addresses a crash when analyzing ELF files with unsupported architectures by adding specific exception handling for vivisect's 'Unsupported Architecture' error. The approach of converting this to a more user-friendly CorruptFile exception is good.
My review includes a suggestion to refactor the exception handling logic in capa/loader.py to make it more robust and prevent potential IndexError crashes within the except block, improving the overall safety of the error handling.
capa/loader.py
Outdated
| if type(e) is Exception: | ||
| error_msg = str(e.args[0]) if e.args else str(e) | ||
|
|
||
| if "Couldn't convert rva" in error_msg: | ||
| raise CorruptFile(e.args[0]) from e | ||
| elif "Unsupported Architecture" in error_msg: | ||
| # Extract architecture number if available | ||
| arch_info = e.args[1] if len(e.args) > 1 else "unknown" | ||
| raise CorruptFile(f"Unsupported architecture: {arch_info}") from e |
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.
This refactoring introduces a potential IndexError. If an exception occurs where e.args is empty, but str(e) contains one of the target error strings (e.g., "Couldn't convert rva"), the code will attempt to access e.args[0] or e.args[1], which will fail.
To make the error handling more robust and cleaner, I suggest checking for e.args upfront. This ensures that you only try to access exception arguments when they exist, preventing a crash in the exception handler. The original exception will be re-raised if e.args is empty, which is the correct fallback behavior.
| if type(e) is Exception: | |
| error_msg = str(e.args[0]) if e.args else str(e) | |
| if "Couldn't convert rva" in error_msg: | |
| raise CorruptFile(e.args[0]) from e | |
| elif "Unsupported Architecture" in error_msg: | |
| # Extract architecture number if available | |
| arch_info = e.args[1] if len(e.args) > 1 else "unknown" | |
| raise CorruptFile(f"Unsupported architecture: {arch_info}") from e | |
| if type(e) is Exception and e.args: | |
| error_msg = str(e.args[0]) | |
| if "Couldn't convert rva" in error_msg: | |
| raise CorruptFile(e.args[0]) from e | |
| elif "Unsupported Architecture" in error_msg: | |
| # Extract architecture number if available | |
| arch_info = e.args[1] if len(e.args) > 1 else "unknown" | |
| raise CorruptFile(f"Unsupported architecture: {arch_info}") from e |
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.
@kami922 please address this change here.
mike-hunhoff
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.
Thank you @kami922 . Please see my review and address the suggested changes.
CHANGELOG.md
Outdated
|
|
||
| ### Bug Fixes | ||
|
|
||
| - loader: gracefully handle ELF files with unsupported architectures @kami922 #2793 |
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 move this to the unreleased section and change the issue number to the number of this PR.
- Add e.args check to prevent IndexError when accessing exception arguments - Use error_msg variable instead of directly accessing e.args[0] - Update CHANGELOG to reference PR mandiant#2800 instead of issue mandiant#2793 Addresses feedback from @mike-hunhoff and gemini-code-assist bot
|
@mike-hunhoff Good day i made the changes as request, one ci was skipped i dont know why tho can you please review thank you! |
mike-hunhoff
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.
Thanks @kami922 , one last change for you to review. Also, please sync with the master branch.
CHANGELOG.md
Outdated
|
|
||
| ### Bug Fixes | ||
|
|
||
| - loader: gracefully handle ELF files with unsupported architectures [email protected] #2800 |
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 move this to the unreleased section of the CHANGELOG, see
Line 3 in dc47de1
| ## master (unreleased) |
|
@mike-hunhoff can i get an update on this one please? Lemme know if there are any more changes to be made |
LGTM, thank you! |
Summary
Fixes #2793 - capa crashes when analyzing ELF files with unsupported architectures
Problem
When vivisect encounters an ELF file with an unsupported architecture (e.g., ARM64 variant), it raises a generic Exception with message
'Unsupported Architecture: %d\n', 183. This was not caught by existing error handlers incapa/loader.py, causing capa to crash with an unfriendly traceback.Solution
Added exception handling in
get_workspace()to detect "Unsupported Architecture" error messages and convert them to user-friendlyCorruptFileexceptions, following the same pattern as the existing "Couldn't convert rva" handler.Changes
Checklist
closes #2793