-
-
Notifications
You must be signed in to change notification settings - Fork 29.2k
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
Zipfile robustness #45223
Comments
One of the ways that zipfile.ZipFile checks for a valid zipfile is by confirming that the specified comment length matches the found comment. I have encountered zipfiles in the wild that indicate a comment length of 0, but have filled the comment area with an arbitrary amount of nulls. In the interests of robustness, I think that "comment = comment.rstrip('\x00') should be added to the zipfile check. |
Alan? |
It would seem that such a zip file is not consistent with the spec |
I agree that the zipfile is out of spec. Here are my arguments in favor Existing zip tools like 7zip, pkzip, and winzip handle these files "as As far as I know, it won't break any valid zipfiles. Because the fix necessary is buried inside a private function in the |
I'd like to piggyback on this issue if okay :D I have some zipfiles I'm working with that contain junk in the extra Example of traceback: >>> zf = zipfile.ZipFile('bad.zip', 'r')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "zipfile.py", line 346, in __init__
self._GetContents()
File "zipfile.py", line 366, in _GetContents
self._RealGetContents()
File "zipfile.py", line 424, in _RealGetContents
x._decodeExtra()
File "zipfile.py", line 267, in _decodeExtra
tp, ln = unpack('<hh', extra[:4])
File "/usr/lib/python2.5/struct.py", line 87, in unpack
return o.unpack(s)
struct.error: unpack requires a string argument of length 4 While I'm working to track down the source of the "extra junk" -- this |
Maybe ZipFile should have an option to handle archives in a non-strict mode, in which it would raise warnings or just completely ignore a small set of minor violations of the spec. That way people that want behavior that's in compliance with the spec will have it (by default), but those that need to handle ZIP files that bend the rules can still choose to use the Python standard libraries to get their work done. If there's any interest in this I can work up a patch, tests, and documentation. |
Yes, I'd be interested in this functionality. Anything that makes it more flexible to use with a wide variety of zipfiles (even bad ones :D) allows for increased reusability. Thanks! |
Note that based on the fact that most zipfile tools handle garbage after the indicated end of the comment by ignoring it, bpo-10694 fixed zipfile to also ignore such trailing data. It sounds like there may be more out-of-spec errors that could be ignored, though. Should this issue therefore be left open? If so, does anyone want to propose a patch? |
So far I haven't had the opportunity to sit down and write up a "lenient zipfile handling" patch; my apologies to those that could really use one. If somebody does propose a patch, I'll be glad to test and review it. I suppose I would like to see the issue kept open for a while, even if just to collect common "bending of the rules" cases that people would like to see supported. |
I'm still affected by this issue. A workaround or patch would be appreciated. |
"This issue" is currently a collection point for specific "allow this in lax mode" issues. Do you have one or more specific cases in point that you'd like to talk about? (Note that the 'garbage after end of file' bug has already been fixed.) |
I received a bug report from a user. He had a zip file created by Mac OS 10.5.8 that the zipfile module claimed was not a valid zip file. The traceback went to function _EndRecData(fpin). The file had a valid comment appended, but recorded a comment length of zero. I am posting to this thread because it seems related. The zero comment length is incorrect, but the file is read without complaint by other software. Note that this is not end of file garbage; the comment is valid. The _EndRecData(fpin) function is reading the "End of Central Directory" record. The endrec[7] is the comment length, but of more interest is endrec[6] which is the start of the Central Directory. This is a file offset, and if valid it points to a Central Directory record. These records start with a known string PK\001\002. I propose that the correct fix is to delete the test for correct comment length, and replace it with a test that reads four bytes at offset endrec[6] and makes sure it is PK\001\002 and a valid record. This is viewed not as a hack for defective software, but rather as an improved sanity check, since finding the Central Directory is vital to reading a zip file. This code replaces the end of _EndRecData(fpin), and was taken from Python2.5 (so check against the relevant version): if start >= 0: # Correct signature string was found
endrec = struct.unpack(structEndArchive, data[start:start+22])
endrec = list(endrec)
comment = data[start+22:]
## Relevant changes here
fpin.seek(endrec[6], 0) # Seek to the start of the central directory
dat = fpin.read(4) # Read four bytes
# Note: Mac OS is known to add a comment, but record the length as zero.
if dat == stringCentralDir: # Success
##
# Append the archive comment and start offset
endrec.append(comment)
endrec.append(filesize - END_BLOCK + start)
if endrec[-4] == -1 or endrec[-4] == 0xffffffff:
return _EndRecData64(fpin, - END_BLOCK + start, endrec)
return endrec
return # Error, return None |
I'm pretty sure that what you are reporting has been fixed by the 'garbage' fix I mentioned (which might better be called the 'more flexible handling of comments' patch). Though not in 2.5 (or 2.6) since they are no longer in maintenance. If you could confirm this by testing against 2.7 or 3.2, that would be great. |
Problem was reported on 2.7. I will check in detail this weekend. Please stand by. |
I grabbed a 2.7.2 zipfile.py, and my original comments stand. If there is a "garbage at end of file" patch, I can't find it; please provide a line number or a hint. The user at yale.edu reports that the patch works. Here is a diff of my changes. To test, append some junk to a good zipfile: echo junk >> good.zip, and try reading it. Let me know if you want me to do anything else; maybe look at 3.2; or email me offline. *** zipfile.py 2011-12-09 11:25:07.000000000 -0500 *** 237,248 **** |
Here's the patch: http://hg.python.org/cpython/rev/cc3255a707c7/ I thought I remembered getting it in to 2.7.2, but my memory is evidently wrong. It has been applied, but is not yet in the released version of 2.7. |
For completeness, I checked other versions of Python. The example zip file fails in Python 3.1, but succeeds in Python 3.2.2. The patch for 3.2.2 removed the check for correct comment length, but substituted no further check for validity. |
I'm assuming that this can now be closed. |
For those who cannot update just yet, see also the workaround at http://stackoverflow.com/a/21996397/874188 |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: