Skip to content
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

Improve PE File & Program Icon Loading #670

Merged
merged 3 commits into from
Jan 12, 2024
Merged

Improve PE File & Program Icon Loading #670

merged 3 commits into from
Jan 12, 2024

Conversation

divadretlaw
Copy link
Contributor

PEFile Improvements

  • Close FileHandle after parsing
  • Split types into multiple files
  • Add some documentation
  • Fix OptionalHeader not differentiating PE32 and PE32+
  • Interpret more values while parsing (maybe a bit overkill, but they could be useful in the future or other projects trying to access values of a PE file)
  • Improve Icon Loading

I think it would be best to cache the icons as it seems to take a bit to load them to find the best / largest one.

I would recommend https://github.com/kean/Nuke but there are many options and even the possibility to create a cache without any dependencies if that is desired.

Adding a dependency should be the decision of the maintainer and if they think adding a cache, and also which library to use if any, then I would be happy to implement that as well.

@IsaacMarovitz
Copy link
Member

Most of the improvements to PE parsing that I'm seeing here are not relevant. The parsing implementation used was designed to be a bit naive, as we really only need the data present in the resource table for our purposes. If we wanted to create a more complete implementation, as is shown here, it might be worth splitting it into a separate project/swift package.

@divadretlaw
Copy link
Contributor Author

That approach makes sense, I reverted the not-needed changes to the PE parsing

@IsaacMarovitz IsaacMarovitz merged commit 643ad89 into Whisky-App:main Jan 12, 2024
1 check 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