-
Notifications
You must be signed in to change notification settings - Fork 481
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
Crashdump writer plugin #718
base: develop
Are you sure you want to change the base?
Crashdump writer plugin #718
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.
Thanks to the clean up of the PR, it's very easy to see the code! 🙂
I left a little comment.
I hope this plugin can be usefully.
@paulkermann LGTM, some of the things you explained were understood. Thank you for checking my reviews. 🙂 |
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.
Mostly awesome, a couple of bits and pieces around hardcoded values but otherwise really good! Nice use of comments and sticking to the existing coding style, thanks! 5:)
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.
Hello, @paulkermann
I'm happy to see that the plugin is getting better through feedback, and I respect your work.
It's cool enough now, but I leave a NIT 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.
Cool, rol
looks good, but bswap
seems to have taken a step backwards, I doesn't feel like it needs two separate methods, and it's not clear why it went from using struct to doing the maths manually, but I think that's the last bit to fix up (still pending a review from someone else from core, but otherwise looking good). 5:)
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.
@paulkermann Thank you, I think the review I can advise is over.. 🙂
I hope the other things will be revised well and the maintainers will like it!
…writer Conflicts: volatility3/framework/layers/physical.py volatility3/framework/plugins/windows/info.py volatility3/framework/symbols/windows/extensions/kdbg.py
@iMHLv2 could you please take a look over the KDBG decoding bits of this? I haven't done a full review yet, but the more eyes we can get on it the better please. 5:) |
Any word on this one guys? |
This is still waiting on review by @awalters I'm afraid. |
from volatility3.framework.renderers import TreeGrid | ||
from volatility3.framework.symbols import intermed | ||
from volatility3.framework.symbols.windows.extensions import kdbg, pe | ||
from volatility3.framework.symbols.windows import extensions |
Check notice
Code scanning / CodeQL
Unused import Note
hey @ikelos, can you merge this plugin? |
The need for review hasn't changed I'm afraid. I don't know the crashdump format well enough to agree to bring this into core and support it long term. |
This is a PR is instead of #694 because the rebase on that PR was a bit weird.
This was tested against 32-bit and 64-bit memory dumps.