Repository navigation
zipfile writes incorrect local file header for large files in zip64 #53929
Description
| BPO | 9720 |
|---|---|
| Nosy | @loewis, @gpshead, @ronaldoussoren, @amauryfa, @tiran, @merwok, @serhiy-storchaka |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2013-01-14.22:49:08.068>
created_at = <Date 2010-08-31.01:02:17.019>
labels = ['type-bug', 'library']
title = 'zipfile writes incorrect local file header for large files in zip64'
updated_at = <Date 2013-01-14.22:49:08.067>
user = 'https://bugs.python.org/craigds'bugs.python.org fields:
activity = <Date 2013-01-14.22:49:08.067>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2013-01-14.22:49:08.068>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2010-08-31.01:02:17.019>
creator = 'craigds'
dependencies = []
files = ['18685', '18780', '27262', '27263', '28145', '28146', '28558', '28559']
hgrepos = []
issue_num = 9720
keywords = ['patch', 'needs review']
message_count = 20.0
messages = ['115250', '115466', '115514', '115660', '115672', '115741', '146923', '156442', '170645', '171010', '171025', '172648', '172652', '175471', '176538', '178603', '179013', '179019', '179987', '179989']
nosy_count = 20.0
nosy_names = ['loewis', 'gregory.p.smith', 'ronaldoussoren', 'amaury.forgeotdarc', 'alanmcintyre', 'christian.heimes', 'nadeem.vawda', 'eric.araujo', 'lambacck', 'dandrzejewski', 'segfault42', 'craigds', 'python-dev', 'enlavin', 'Paul', 'serhiy.storchaka', 'Kristof.Keppens', 'Ruben.Gonzalez', 'jhenry82', 'Nico.M\xc3\xb6ller']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue9720'
versions = ['Python 2.7', 'Python 3.2', 'Python 3.3', 'Python 3.4']
Activity
craigds commentedon Aug 31, 2010
Steps to reproduce:
Now inspect the file headers using a hex editor. The written headers are incorrect. The filesize and compressed size should be written as 0xffffffff and the 'extra field' should contain the actual sizes.
Tested on Python 2.5 but looking at the latest code in 3.2 it still looks broken.
The problem is that the ZipInfo.FileHeader() is written before the filesize is populated, so Zip64 extensions are not written. Later, the sizes in the header are written, but Zip64 extensions are not taken into account and the filesize is just wrapped (7gb becomes 3gb, for instance).
My patch fixes the problem on Python 2.5, it might need minor porting to fix trunk. It works by assigning the uncompressed filesize to the ZipInfo header initially, then writing the header. Then later on, I re-write the header (this is okay since the header size will not have increased.)
merwok commentedon Sep 3, 2010
A tip about versions: Development happens on the current active branch, py3k (future 3.2 version), and bug or doc fixes are backported to the stable versions 2.7 and 3.1. Security fixes go into 2.6 too.
Can you reproduce your bug in 2.7, 3.1 and 3.2?
Adding Alan to nosy since he’s listed in Misc/maintainers.rst.
craigds commentedon Sep 3, 2010
Yes, the bug still exists in Python 3.1.2. However, struct.pack() no longer silently ignores overflow, so I get this error instead:
>>> z.write('foo.txt') Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/usr/lib/python3.1/zipfile.py", line 1095, in write zinfo.file_size)) struct.error: argument out of rangealanmcintyre commentedon Sep 5, 2010
Thanks for the patch, Craig; I should have some time later today or tomorrow to do a review. Did you have a patch for the test suite(s) as well? If not, I can just make sure your test case is covered in test_zipfile64.
craigds commentedon Sep 5, 2010
Hi, sorry no I haven't had time to add a real test for this
alanmcintyre commentedon Sep 7, 2010
Here's an updated patch for the py3k trunk with tests. This pretty much doubles the runtime of test_zipfile64.py. The patch also removes some unnecessary code from the existing test_zipfile64 tests.
Note: It looks like writestr will also suffer from a struct.pack overflow if it's given a ZipInfo with the third general purpose flag bit set. I won't have time to address that until next weekend, probably.
nadeemvawda commentedon Nov 3, 2011
bpo-6434 was marked as a duplicate of this issue.
serhiy-storchaka commentedon Mar 20, 2012
I am afraid that the problem is more complicated. With the option allowZip64=True all files need to write with this extension, because size of local file header may change and there will be after compression just go back and rewrite it.
Now it appears that the Zip64 option simply does not work.
tiran commentedon Sep 18, 2012
Serhiy:
If I understand you correctly it should be easy to fix. The code in close() has to check if any file is beyond the ZIP64 limit and then write all headers with extra args. Is that correct?
serhiy-storchaka commentedon Sep 22, 2012
No, on the contrary, it is not such easy to fix, and the patch is incorrect.
Sorry that it is not clear either. The size of the header with extra args
depends on the size of the file. The file size can be changed in the process of
compressing, and compressed size may be larger than uncompressed size,
exceeding 32-bit boundary. Rewriting the header with extra args, we can
overwrite compressed data.
I was put off the issue for further more careful research. Thanks for the
reminder.
One solution is always (even for smallest files) to write 64-bit sizes when
allowZip64 is true.
serhiy-storchaka commentedon Sep 23, 2012
I see two rational solutions of the issue (all written below is applicable only for allowZip64=True):
The first patch (zipfile_zip64_always.patch) uses this approach.
The second patch (zipfile_zip64_try.patch) is based on Alan's patch and uses the second approach. The probability of errors is reduced and they are now detected and does not lead to a silent data damage.
Both patches are for Python 3.3. If any patch is good, I'll backport it for the older versions.
serhiy-storchaka commentedon Oct 11, 2012
What the conclusion about the patches? Which variant I should backport for older versions?
ronaldoussoren commentedon Oct 11, 2012
I'd write the extended header when the current file size is larger than the zip64 limit (that is, when 'st.st_size > ZIP64_LIMIT' in the write method.
That way the minimal header size is used whenever possible.
As you noted this can cause problems when the file grows beyond the limit while it is stored in the zipfile, but IMHO storing data while it is modified is asking for problems anyway.
BTW. I haven't actually review the patch yet.
serhiy-storchaka commentedon Nov 12, 2012
Please, review the patches.
serhiy-storchaka commentedon Nov 28, 2012
Patches updated to resolve merge conflict with bpo-11981.
Please review and apply any of this patches. This is needed for some
other my zipfile patches.
serhiy-storchaka commentedon Dec 30, 2012
What variant of patches should I commit? Or prepare other?
NicoMller commentedon Jan 4, 2013
I most definitely need a patch for 2.7.3
Would be awesome if you could provide a patch for that version.
serhiy-storchaka commentedon Jan 4, 2013
Here are second variant patches for 2.7 and 3.2.
python-dev commentedon Jan 14, 2013
New changeset ce869b05762c by Serhiy Storchaka in branch '2.7':
Issue bpo-9720: zipfile now writes correct local headers for files larger than 4 GiB.
http://hg.python.org/cpython/rev/ce869b05762c
New changeset b93848ca7760 by Serhiy Storchaka in branch '3.2':
Issue bpo-9720: zipfile now writes correct local headers for files larger than 4 GiB.
http://hg.python.org/cpython/rev/b93848ca7760
New changeset 656a45738e5e by Serhiy Storchaka in branch '3.3':
Issue bpo-9720: zipfile now writes correct local headers for files larger than 4 GiB.
http://hg.python.org/cpython/rev/656a45738e5e
New changeset 628a6af64a46 by Serhiy Storchaka in branch 'default':
Issue bpo-9720: zipfile now writes correct local headers for files larger than 4 GiB.
http://hg.python.org/cpython/rev/628a6af64a46
serhiy-storchaka commentedon Jan 14, 2013
Fixed. Thank you for report, Craig de Stigter.