zipfile.ZipFile() unable to open zip File with a short extra header

[pleed]

BPO	14315
Nosy	@loewis, @terryjreedy, @gpshead, @mdickinson, @merwok, @meadori, @serhiy-storchaka
Files	bla.apk: APK File (Zip Format) that unzips but crashes when opened with zipfile fix_zipfile_extra.patch: FIX: Ignore extra padding bytes in zip file zipfile_extra_test.patch: Test for zipfile with padding.

^{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/gpshead'
closed_at = <Date 2014-05-30.06:59:38.064>
created_at = <Date 2012-03-15.11:15:45.559>
labels = ['type-bug', 'library']
title = 'zipfile.ZipFile() unable to open zip File with a short extra header'
updated_at = <Date 2018-08-06.06:43:30.242>
user = 'https://bugs.python.org/pleed'

bugs.python.org fields:

activity = <Date 2018-08-06.06:43:30.242>
actor = 'serhiy.storchaka'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2014-05-30.06:59:38.064>
closer = 'gregory.p.smith'
components = ['Library (Lib)']
creation = <Date 2012-03-15.11:15:45.559>
creator = 'pleed'
dependencies = []
files = ['24860', '24902', '25564']
hgrepos = []
issue_num = 14315
keywords = ['patch']
message_count = 31.0
messages = ['155874', '156089', '156110', '156135', '156147', '159584', '159602', '160538', '160539', '160541', '160550', '160553', '160590', '160591', '160645', '160655', '160663', '160664', '160673', '160684', '163743', '163749', '163813', '163831', '163835', '163855', '163857', '187177', '219377', '219378', '323179']
nosy_count = 12.0
nosy_names = ['loewis', 'terry.reedy', 'gregory.p.smith', 'mark.dickinson', 'alanmcintyre', 'eric.araujo', 'ubershmekel', 'meador.inge', 'python-dev', 'serhiy.storchaka', 'pleed', 'void.sender']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue14315'
versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

[pleed]

zipfile.ZipFile("bla.apk") crashes, but should not since other tools work perfectly with this file.

Python 2.7.2+ (default, Oct  4 2011, 20:06:09) 
[GCC 4.6.1] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import zipfile
>>> zipfile.ZipFile("bla.apk")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.7/zipfile.py", line 710, in __init__
    self._GetContents()
  File "/usr/lib/python2.7/zipfile.py", line 744, in _GetContents
    self._RealGetContents()
  File "/usr/lib/python2.7/zipfile.py", line 803, in _RealGetContents
    x._decodeExtra()
  File "/usr/lib/python2.7/zipfile.py", line 369, in _decodeExtra
    tp, ln = unpack('<HH', extra[:4])
struct.error: unpack requires a string argument of length 4

[terryjreedy]

.apk is primarily used for Android Package files
https://en.wikipedia.org/wiki/APK_%28file_format%29
which are zipped JAR-based archives. By changing .apk to .zip, I can open the file in Win7 explorer, which has zip built in.

A 'crash' is a segfault or equvalent. A traceback is a graceful shutdown and not a crash. I reproduced the traceback on 3.3.0a1. It appears to say that there are not enough bytes.

import struct
struct.unpack('<HH', b'abc')
#produces the same error
struct.error: unpack requires a bytes object of length 4

The 3.x message is clearer in that the second arg is a bytestring, not a char string. It would not hurt it it said how many bytes it did get.

However, the behavior that leads to the message baffles me. The _decodeExtra code looks like this, with some prints that I added.

    def _decodeExtra(self):
        # Try to decode the extra field.
        extra = self.extra
        unpack = struct.unpack
        while extra:
            print(extra)
            tp, ln = unpack('<HH', extra[:4])
            print(tp,ln)
            if tp == 1: 
                pass  # substitute for actual code not called
            extra = extra[ln+4:]
            print(extra)

The output is
b'\xfe\xca\x00\x00'
51966 0
b''
b'\x00'
Traceback (most recent call last): ...

So it looks like extra was properly formatted and properly snipped to 0 bytes, but somehow gained a null byte before being tested again. 1 byte is not 4 bytes and hence the error, but how?

[serhiy-storchaka]

This is print from *other* _decodeExtra call. Add print('start') at start of _decodeExtra and you will see it.

start
b'\xfe\xca\x00\x00'
51966 0
b''
start
start
b'\x00'

[terryjreedy]

Thanks. There must be multiple extra fields allowed. This output suggests that a) zipfile mis-parses the extra field in some peculiar case (which I doubt) or b) the file either gives a bad field length or third extra field is mal-formed and that other tools catch the error and ignore the bad extra field whereas zipfile does not.

[serhiy-storchaka]

This is padding. Other implimentations just ignore it and Python must.

[serhiy-storchaka]

Anyone can look at the patch? This same problem reported in http://bugs.python.org/msg73317 .

[ubershmekel]

If we're modifying zipfile, please consider adding the reviewed patch for ZipFile.remove at http://bugs.python.org/issue6818

[loewis]

Serhiy: can you please provide a unit test? The OP's test case is unsuitable because of both size and copyright.

As for the actual issue: the extra data (type 0xcafe) is apparently added by jar tools to make the jar file executable:

http://www.androidadb.com/source/commons-compress-1.1-src/src/main/java/org/apache/commons/compress/archivers/zip/JarMarker.java.html

ISTM that the extra 0 byte is in clear violation of the ZIP specification, which says that there always be type+length headers, followed by length data. So if the length is 0, there ought not to be any data. I don't buy the "padding" argument, since a) the spec is silent on any padding, and b) for alignment reasons, it's not plausible to add any padding, since it is aligned fine without this padding, and unaligned with the padding.

I can sympathize with the desire to accept the zipfile, anyway (i.e. despite it being broken). At the same time, I also think that Python should not let errors pass silently.

So as a way out, I propose that the ZipFile class gains a "strict" attribute, indicating whether "acceptable" violations of the spec are ignored or reported as exceptions.

So -1 on the patch as it stands (but thanks for the analysis).

[serhiy-storchaka]

Serhiy: can you please provide a unit test? The OP's test case is unsuitable because of both size and copyright.

I provided the test several minutes ago.

[serhiy-storchaka]

I can sympathize with the desire to accept the zipfile, anyway (i.e. despite it being broken). At the same time, I also think that Python should not let errors pass silently.

I do not know other implementation of ZIP, which output an error or a
warning on such files. The fact is that such files exist in the wild
world.

So as a way out, I propose that the ZipFile class gains a "strict" attribute, indicating whether "acceptable" violations of the spec are ignored or reported as exceptions.

It is a not easy task (and unnecessary, I suppose). Now zipfile ignores
many errors (for example, it completely ignores local file headers).

[loewis]

> So as a way out, I propose that the ZipFile class gains a "strict"
> attribute, indicating whether "acceptable" violations of the spec
> are ignored or reported as exceptions.

It is a not easy task (and unnecessary, I suppose). Now zipfile
ignores many errors (for example, it completely ignores local file
headers).

Why do you consider this difficult? Just add a "strict" flag, make
it false by default, and raise an error if there is any unparsable
extra data.

I'm not asking that you implement a complete validity check for all
aspects of the zip spec - just that there is a mode where it continues
to raise an exception as it currently does (but with a better
exception).