Skip to content

Initial commit for PE parser#28

Merged
Schamper merged 10 commits intomainfrom
pe
Jul 31, 2025
Merged

Initial commit for PE parser#28
Schamper merged 10 commits intomainfrom
pe

Conversation

@Schamper
Copy link
Copy Markdown
Member

@Schamper Schamper commented Jul 14, 2025

While reviewing #10 it quite quickly became apparent that it was more beneficial to create a new parsing base to work from. This PR implements that base. This means that almost all code here is made with the intent to parse, not to write. The idea is that instead of trying to wreck our brain on how to create a nice interface to both read and write, we first focus on making a nice parsing/reading interface, and to take the lessons learned from #10 regarding writing and at some later point apply them to this new base.

Notably this base focuses on:

No hard feelings to anyone involved with #10. Love you @sud0woodo.

Todo for this PR:

  • Unit tests
  • Implement bound import directory

Depends on #29 for some minor project cleanup.

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 14, 2025

Codecov Report

❌ Patch coverage is 79.45708% with 280 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.90%. Comparing base (e312272) to head (b5e33d5).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
dissect/executable/pe/directory/resource.py 75.52% 81 Missing ⚠️
dissect/executable/pe/pe.py 74.33% 68 Missing ⚠️
dissect/executable/pe/directory/delay_import.py 76.19% 25 Missing ⚠️
dissect/executable/pe/directory/debug.py 82.85% 24 Missing ⚠️
dissect/executable/pe/directory/imports.py 76.92% 21 Missing ⚠️
dissect/executable/pe/directory/export.py 80.68% 17 Missing ⚠️
dissect/executable/pe/directory/bound_import.py 84.05% 11 Missing ⚠️
dissect/executable/pe/directory/exception.py 80.00% 6 Missing ⚠️
dissect/executable/pe/directory/load_config.py 83.33% 6 Missing ⚠️
dissect/executable/pe/directory/com_descriptor.py 89.36% 5 Missing ⚠️
... and 5 more
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #28       +/-   ##
===========================================
- Coverage   92.33%   81.90%   -10.44%     
===========================================
  Files           5       24       +19     
  Lines         300     1663     +1363     
===========================================
+ Hits          277     1362     +1085     
- Misses         23      301      +278     
Flag Coverage Δ
unittests 81.90% <79.45%> (-10.44%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Schamper Schamper force-pushed the pe branch 6 times, most recently from 0b7f414 to 0fe80b8 Compare July 16, 2025 12:44
Copy link
Copy Markdown
Contributor

@Miauwkeru Miauwkeru left a comment

Choose a reason for hiding this comment

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

I am a bit torn on passing the PE object to every directory and section etc, while in most cases passing just the file handle will do (with some additional flags). This makes it a bit harder in the future to test specific parts without creating a PE object first as it is strongly coupled to the PE object.

But then again, how often would we encounter a file or a set of bytes where we know it is a BaseReloactionDirectory for example, and just want to parse that part without the PE file.

So we could leave it as it is and only change it when it is actually needed. aka, if we want to parse directories or sections without a PE file.

@Schamper
Copy link
Copy Markdown
Member Author

I am a bit torn on passing the PE object to every directory and section etc, while in most cases passing just the file handle will do (with some additional flags). This makes it a bit harder in the future to test specific parts without creating a PE object first as it is strongly coupled to the PE object.

But then again, how often would we encounter a file or a set of bytes where we know it is a BaseReloactionDirectory for example, and just want to parse that part without the PE file.

So we could leave it as it is and only change it when it is actually needed. aka, if we want to parse directories or sections without a PE file.

Cross that bridge when we get there.

Schamper and others added 2 commits July 18, 2025 10:58
Co-authored-by: Miauwkeru <Miauwkeru@users.noreply.github.com>
@Schamper Schamper requested a review from Miauwkeru July 24, 2025 15:17
@Schamper Schamper requested a review from Miauwkeru July 30, 2025 11:55



/* TODO */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is this TODO here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Because there are a ton of structures and enums related to symbols and dynamic relocations that aren't in this file yet but don't need to be in there for now. Can be a future improvement.

@Schamper Schamper requested a review from Miauwkeru July 30, 2025 16:56
Copy link
Copy Markdown
Contributor

@Miauwkeru Miauwkeru left a comment

Choose a reason for hiding this comment

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

From what I have read OS2 is a separate executable format so it doesn't fit inside the PE class. So while good enough for now, if we ever want to fully implement the OS2 format we should move it to a different class. If that case ever happens

@Schamper
Copy link
Copy Markdown
Member Author

From what I have read OS2 is a separate executable format so it doesn't fit inside the PE class. So while good enough for now, if we ever want to fully implement the OS2 format we should move it to a different class. If that case ever happens

I know, that's why I also didn't build it out any further.

@Schamper Schamper merged commit 6870758 into main Jul 31, 2025
23 of 25 checks passed
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.

2 participants