loader: handle struct.error from dnfile and raise CorruptFile with a clear message#2872
loader: handle struct.error from dnfile and raise CorruptFile with a clear message#2872devs6186 wants to merge 2 commits intomandiant:masterfrom
Conversation
Summary of ChangesHello @devs6186, 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 capa's .NET file analysis by gracefully handling corrupted or malformed .NET metadata. Previously, such files could lead to unhandled 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. Changelog
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.
Code Review
The pull request effectively addresses the struct.error raised by dnfile.dnPE() by wrapping the call in a try/except block and re-raising a CorruptFile exception with a clear, user-friendly message. This improves error handling and provides better feedback for malformed .NET PE files. The changes are applied consistently across both capa/features/extractors/dnfile/extractor.py and capa/features/extractors/dotnetfile.py, and the CHANGELOG.md has been updated appropriately. The addition of import struct is also correct.
|
Thanks for the review! Glad the approach looks right. One small thing worth calling out on the No other changes needed based on the feedback — both extractors are now consistent in how they surface this error to the caller. |
|
Would a helper function make sense to only write the except handling code once? |
Well both suggestions make sense and I can incorporate them.
|
|
@devs6186 CI tests are failing: Please ensure all unit tests are passing locally before requesting another review, thank you! |
yes @mike-hunhoff i just noticed, ill take a days time not more than that and look into all the comments and changes requested for this pr and all the others and get back to you asap, thank you for your patience ! |
…ssage When .NET metadata is truncated or invalid, dnfile can raise struct.error. Catch it in DnfileFeatureExtractor and DotnetFileFeatureExtractor and re-raise as CorruptFile with a user-friendly message. Fixes mandiant#2442
c4b0563 to
da59237
Compare
@devs6186 we'll give this another review once you've addressed these changes, thank you! |
Add load_dotnet_image() to dnfile/helpers.py that calls dnfile.dnPE()
and catches struct.error, raising CorruptFile with the original error
message included (f"Invalid or truncated .NET metadata: {e}").
Both DnfileFeatureExtractor and DotnetFileFeatureExtractor now call the
helper instead of duplicating the try/except block, and their direct
import of struct is removed.
Addresses review feedback on mandiant#2872.
i have addressed them now in the latest commit |
|
@devs6186 lints are failing. Please ensure all lints pass locally before requesting another review. |
closes #2442
When capa analyzes a .NET PE with truncated or malformed .NET metadata,
dnfile.dnPE()raisesstruct.error: unpack_from requires a buffer of at least N bytes. This is caught nowhere and bubbles up as an unhandled exception with no useful user-facing message.Changes
capa/features/extractors/dnfile/extractor.pyandcapa/features/extractors/dotnetfile.py, wrappeddnfile.dnPE(...)intry/except struct.error.capa.loader.CorruptFilewith the message: "Invalid or truncated .NET metadata; the file may be corrupted or not a valid .NET PE."import structto both files.Checklist