Skip to content

Conversation

Nitr0-G
Copy link

@Nitr0-G Nitr0-G commented Feb 4, 2024

Added the ability to read files larger than 40MB at a time. This is necessary, for example, for further initialization of a large PE file, for example, its Imports and the like, which requires a full file in memory. One bool variable was added with default initialization as false, so as not to disrupt the work of projects on older versions or when switching to a new version. The file is loaded by linking two APIs: VirtualAlloc + ReadFile. Thanks for such a wonderful pe parser like this =)

Added the ability to read files larger than 40MB at a time. This is necessary, for example, for further initialization of a large PE file, for example, its Imports and the like, which requires a full file in memory. One bool variable was added with default initialization as false, so as not to disrupt the work of projects on older versions or when switching to a new version. The file is loaded by linking two APIs: VirtualAlloc + ReadFile. Thanks for such a wonderful pe parser like this =)
@Nitr0-G Nitr0-G requested a review from woodruffw as a code owner February 4, 2024 00:23
@CLAassistant
Copy link

CLAassistant commented Feb 4, 2024

CLA assistant check
All committers have signed the CLA.

Comment on lines +256 to +277

LPVOID ptr = nullptr;

if (!LargeFile) {
ptr = MapViewOfFile(hMap, FILE_MAP_READ, 0, 0, 0);
} else {
ptr = VirtualAlloc(NULL, fileSize, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);
if (ptr == INVALID_HANDLE_VALUE) {
CloseHandle(h);
CloseHandle(ptr);
return nullptr;
}

const bool bFileRead = ReadFile(h, ptr, fileSize, nullptr, nullptr);
if(!bFileRead) {
CloseHandle(h);
if (ptr != nullptr) {
CloseHandle(ptr);
}

return nullptr;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it only uses a mmap backing on Windows, but we'll probably want similar behavior on other OSes as well. Could you add that to this changeset?


// get a PE parse context from a file
parsed_pe *ParsePEFromFile(const char *filePath);
parsed_pe *ParsePEFromFile(const char *filePath, bool LargeFile = false);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a weak -1 on modifying the API like this: IMO we should pick an arbitrary-ish size for the "large file" cutoff, and use stat or ftell or similar to determine when to switch over to it based on the user input.

(We could do this via a macro, so users who compile pe-parse themselves could customize it.)

Copy link
Contributor

Choose a reason for hiding this comment

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

32MB seems like a reasonable starting point for the cutoff.

@woodruffw
Copy link
Contributor

Thanks for these changes @Nitr0-G! I've given them an initial review pass and left some thoughts.

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.

3 participants