Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-103861: Fix Zip64 extensions not being properly applied in some cases#103863
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
pR0Ps commented Apr 26, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
gpshead commented Apr 26, 2023
I think this is the issue you were talking about at lunch today? (yay pycon sprints!) |
pR0Ps commented Apr 26, 2023
Yep, that's this one! |
bd02290 to 03a58a6CompareThis commit fixes an issue where adding a small file to a `ZipFile` object while forcing zip64 extensions causes an extra Zip64 record to be added to the zip, but doesn't update the `min_version` or file sizes. Fixespython#103861
This fixes an issue where if data requiring zip64 extensions was added to an unseekable stream without specifying `force_zip64=True`, zip64 extensions would not be used and a RuntimeError would not be raised when closing the file (even though the size would be known at that point). This would result in successfully writing corrupt zip files. Deciding if zip64 extensions are required outside of the `FileHeader` function means that both `FileHeader` and `_ZipWriteFile` will always be in sync. Previously, the `FileHeader` function could enable zip64 extensions without propagating that decision to the `_ZipWriteFile` class, which would then not correctly write the data descriptor record or check for errors on close.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
pR0Ps commented May 11, 2023
@gpshead Addressed review comments |
…ompatibility in the API. Code within this module always passes an explicit zip64, so the overall bug fix remains valid. We just don't want to break existing user code constructing their own ZipInfo objects and calling zi.FileHeader themselves for whatever reasons. _(hopefully rare, but it isn't a protected or private API so we can't make assumptions)_
gpshead commented May 16, 2023
github actions CI infrastructure seems broken at the moment. But I think this the PR is ready. I undid one part of your change so it'd remain a pure bugfix: ZipInfo.FileHeader() is technically a public API - even if it seems like nothing should use it we must assume someone does. So getting rid of the old I'll take another look later and rerun CI after it's healthy again. |
miss-islington commented May 16, 2023
miss-islington commented May 16, 2023
Sorry, @pR0Ps and @gpshead, I could not cleanly backport this to |
…ed in some cases (pythonGH-103863) Fix Zip64 extensions not being properly applied in some cases: Fixes an issue where adding a small file to a `ZipFile` object while forcing zip64 extensions causes an extra Zip64 record to be added to the zip, but doesn't update the `min_version` or file sizes in the primary central directory header. Also fixed an edge case in checking if zip64 extensions are required: This fixes an issue where if data requiring zip64 extensions was added to an unseekable stream without specifying `force_zip64=True`, zip64 extensions would not be used and a RuntimeError would not be raised when closing the file (even though the size would be known at that point). This would result in successfully writing corrupt zip files. Deciding if zip64 extensions are required outside of the `FileHeader` function means that both `FileHeader` and `_ZipWriteFile` will always be in sync. Previously, the `FileHeader` function could enable zip64 extensions without propagating that decision to the `_ZipWriteFile` class, which would then not correctly write the data descriptor record or check for errors on close. If anyone is actually using `ZipInfo.FileHeader` as a public API without explicitly passing True or False in for zip64, their own code may still be susceptible to that kind of bug unless they make a similar change to where the zip64 decision happens. FixespythonGH-103861 --------- Co-authored-by: Gregory P. Smith <greg@krypto.org>. (cherry picked from commit 798bcaa) Co-authored-by: Carey Metcalfe <carey@cmetcalfe.ca>
bedevere-bot commented May 16, 2023
GH-104534 is a backport of this pull request to the 3.11 branch. |
* main: pythonGH-104510: Fix refleaks in `_io` base types (python#104516) pythongh-104539: Fix indentation error in logging.config.rst (python#104545) pythongh-104050: Don't star-import 'types' in Argument Clinic (python#104543) pythongh-104050: Add basic typing to CConverter in clinic.py (python#104538) pythongh-64595: Fix write file logic in Argument Clinic (python#104507) pythongh-104523: Inline minimal PGO rules (python#104524) pythongh-103861: Fix Zip64 extensions not being properly applied in some cases (python#103863) pythongh-69152: add method get_proxy_response_headers to HTTPConnection class (python#104248) pythongh-103763: Implement PEP 695 (python#103764) pythongh-104461: Run tkinter test_configure_screen on X11 only (pythonGH-104462) pythongh-104469: Convert _testcapi/watchers.c to use Argument Clinic (python#104503) pythongh-104482: Fix error handling bugs in ast.c (python#104483) pythongh-104341: Adjust tstate_must_exit() to Respect Interpreter Finalization (pythongh-104437) pythonGH-102613: Fix recursion error from `pathlib.Path.glob()` (pythonGH-104373)
…some cases (GH-103863) (#104534) Fix Zip64 extensions not being properly applied in some cases: Fixes an issue where adding a small file to a `ZipFile` object while forcing zip64 extensions causes an extra Zip64 record to be added to the zip, but doesn't update the `min_version` or file sizes in the primary central directory header. Also fixed an edge case in checking if zip64 extensions are required: This fixes an issue where if data requiring zip64 extensions was added to an unseekable stream without specifying `force_zip64=True`, zip64 extensions would not be used and a RuntimeError would not be raised when closing the file (even though the size would be known at that point). This would result in successfully writing corrupt zip files. Deciding if zip64 extensions are required outside of the `FileHeader` function means that both `FileHeader` and `_ZipWriteFile` will always be in sync. Previously, the `FileHeader` function could enable zip64 extensions without propagating that decision to the `_ZipWriteFile` class, which would then not correctly write the data descriptor record or check for errors on close. If anyone is actually using `ZipInfo.FileHeader` as a public API without explicitly passing True or False in for zip64, their own code may still be susceptible to that kind of bug unless they make a similar change to where the zip64 decision happens. FixesGH-103861 --------- . (cherry picked from commit 798bcaa) Co-authored-by: Carey Metcalfe <carey@cmetcalfe.ca>
This commit fixes an issue where adding a small file to a
ZipFileobject while forcing zip64 extensions causes an extra Zip64 record to be added to the zip, but doesn't update themin_versionor file sizes.To create a file that reproduces the issue (copied from #103861):
Diff of information extracted by zipdetails from running the above script before and after this commit.
A test has also been added that checks that these properties are correctly set.
Potential reviewer based on the git blame of the changed lines: @serhiy-storchaka (182d7cd)
Potential reviewers based on the experts index: @Yhg1s, @gpshead