Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-121171: Fix zip64 extract version in local headers#121177
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
If the total archive size or file count in the archive exceeded thezip64 thresholds, the zip64 minimum extract version was not beingwritten to the local header. The central header had the correct version
@jaraco are you able to do a review on this bug fix? Thanks! |
| @@ -1733,6 +1733,12 @@ def _open_to_write(self, zinfo, force_zip64=False): | |||
| if self._seekable: | |||
| self.fp.seek(self.start_dir) | |||
| zinfo.header_offset = self.fp.tell() | |||
| # exceptions raised in _writecheck if _allowZip64 is False | |||
| zip64 = ( | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
you could|= here and save a line
| # exceptions raised in _writecheck if _allowZip64 is False | ||
| zip64 = ( | ||
| zip64 | ||
| or zinfo.header_offset > ZIP64_LIMIT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I explicitly approve of the following line being>= because it makes having the extra unambiguous, you might make this line also>= for the same reason. I think the functionality lost by being able to produce this precise file with_allowZip64=False is minimal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't know if this was copypasted from the central directory logic; it would be nice if they match as this is fixing the inconsistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, I copied the conditionals from:
cpython/Lib/zipfile/__init__.py
Lines 1860 to 1868 in182e035
| iflen(self.filelist)>=ZIP_FILECOUNT_LIMIT: | |
| requires_zip64="Files count" | |
| elifzinfo.file_size>ZIP64_LIMIT: | |
| requires_zip64="Filesize" | |
| elifzinfo.header_offset>ZIP64_LIMIT: | |
| requires_zip64="Zipfile size" | |
| ifrequires_zip64: | |
| raiseLargeZipFile(requires_zip64+ | |
| " would require ZIP64 extensions") |
(filesize is handled a few lines above the added code)
This is the line with the different operator that existed before this PR:
cpython/Lib/zipfile/__init__.py
Line 2065 in182e035
| ifcentDirCount>ZIP_FILECOUNT_LIMIT: |
Good catch. I'd be happy either way as long as we use the same one everywhere.
GivenZIP_FILECOUNT_LIMIT = (1 << 16) - 1 (1 less than the actual limit of0xFFFF) I would be inclined to change if to> to be consistent with how all the other checks againstZIP64_LIMIT andZIP_FILECOUNT_LIMIT are written (we also get to cram one more file in :p) but I don't feel that strongly about it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't follow the rest of the logic,
>>> hex((1<<16) - 1)'0xffff'There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ah whoops.(1<<16) - 1 is not one less than the limit - it is the limit :p
I still have a small preference that it should be changed to> so that we can store the maximum possible files allowed without the zip64 extra field and for consistency with all the other checks againstZIP64_LIMIT andZIP_FILECOUNT_LIMITZIP_MAX_COMMENT that use>.
Uh oh!
There was an error while loading.Please reload this page.
Fix: Wrong extract version set in local headers for files located beyond zip64 limit#121171