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

256 MiB maximum offset limit is too low #396

Open
randomascii opened this issue Mar 18, 2024 · 1 comment
Open

256 MiB maximum offset limit is too low #396

randomascii opened this issue Mar 18, 2024 · 1 comment

Comments

@randomascii
Copy link

The official dcheck enabled builds for Chrome are huge and have maximum section offsets of around 320 MB which is beyond the arbitrary hard-coded limit of 256 MiB in pefile.py. This causes symbol uploads to fail for these builds.

The limit should be increased to avoid this, and arguably the handling of the maximum should be changed so that an exception is thrown because the warning messages that are printed when invoking pefile.py are suppressed or otherwise hidden when using pefile.py as an import.

I have created a local change for Chrome that centralizes and increases the limit to 1 GiB.

aarongable pushed a commit to chromium/chromium that referenced this issue Mar 18, 2024
In an attempt to detect corrupt files pefile.py imposes an arbitrary
maximum offset of 0x10000000 (256 MiB). However Chrome's dcheck official
builds exceed that limit which means that uploading of symbols started
failing. This change increases the limit to 1 GiB.

An issue has been filed against the pefile project for a long-term fix
but this fix will unblock dcheck official builds for now.

erocarrera/pefile#396

Bug: 329661971
Change-Id: Ica4905a61216a11e4ad56734977582133bc26684
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5377931
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1274350}
@randomascii
Copy link
Author

The change I made in Chromium's copy of pefile.py can be found here:

https://chromium-review.googlesource.com/c/chromium/src/+/5377931

It's pretty simple - I just centralized the maximum offset limit and then raised it. I could easily make a PR for this change for here, if that would be accepted.

Or, maybe the limit should just be removed - maybe it has served its purpose and no longer makes sense.

Or, maybe the limit should be a constructor parameter. I haven't checked to see how much work this would be but at a high level this would be a cleaner solution.

Finally, if the limit is reached then I think an exception should be thrown so that pefile.py doesn't fail in silent and mysterious ways.

Any thoughts on this before I create a PR?

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

No branches or pull requests

1 participant