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

.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?

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'

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.

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

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

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

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: 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.

> 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).

>> 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).

> Just add a "strict" flag, make
> it false by default, and raise an error if there is any unparsable
> extra data.

If the module does not actually checks the correctness of all
(including local file headers, which now it ignores), it will be a false
promise. And I don't understand who these checks are needed at all.

Note that the parameter "strict" in htmlparser is considered to be
obsolete.

This is definitely *not* a padding issue.
The problem is that one of the records has an extra field of length 1:

$ zipinfo -v bla.apk

...

Central directory entry #3:
---------------------------

  There are an extra 16 bytes preceding this file.

  res/raw/ice.png

  offset of local header from start of archive:     1190 (000004A6h) bytes
  file system or operating system of origin:        MS-DOS, OS/2 or NT FAT
  version of encoding software:                     1.0
  minimum file system compatibility required:       MS-DOS, OS/2 or NT FAT
  minimum software version required to extract:     1.0
  compression method:                               none (stored)
  file security status:                             not encrypted
  extended local header:                            no
  file last modified on (DOS date/time):            2012 Jan 13 15:30:08
  32-bit CRC value (hex):                           f969abab
  compressed size:                                  26250 bytes
  uncompressed size:                                26250 bytes
  length of filename:                               15 characters
  length of extra field:                            1 bytes
  length of file comment:                           0 characters
  disk number on which file begins:                 disk 1
  apparent file type:                               binary
  non-MSDOS external file attributes:               000000 hex
  MS-DOS file attributes (00 hex):                  none

  The central-directory extra field contains:

  There is no file comment.


Our implementation can't deal with that because of the line that Terry 
pointed out:

            tp, ln = unpack('<HH', extra[:4])

As Martin pointed out, the standard says that things must be in 
multiples of 4-bytes.  So the record is non-portable.

FWIW, I think it will OK to just ignore extra fields that we can't 
interpret as according to the standard.  The only one we currently
care about is the "Zip64 extended information extra field".  Also,
other programs (including the Info-Zip tools) seem to mostly ignore
these fields.

> This is definitely *not* a padding issue.

This is definitely a padding issue. All uncompressed files are located
so that the data starts with a 4-byte boundary (1190+30+15+1=1236, 27486
+30+17+3=27536, etc). This is, probably, allows the use of mmap for the
resources.

> As Martin pointed out, the standard says that things must be in 
> multiples of 4-bytes.

More precisely, the extra field must have at least 4-bytes length to fit
a header. The standard is insufficiently defined in terms of what would
happen if the rest of the field is less than 4 bytes (this is hidden
behind by ellipsis).

>   So the record is non-portable.

De jure the record is non-portable. De facto the record is portable
(many other tools supports it). But even if it does not portable, we are
dealing with the expansion of the zip format, which is very easy support
for reading.

On Mon, May 14, 2012 at 12:31 PM, Serhiy Storchaka
<report@bugs.python.org> wrote:

> Serhiy Storchaka <storchaka@gmail.com> added the comment:
>
>> This is definitely *not* a padding issue.
>
> This is definitely a padding issue. All uncompressed files are located
> so that the data starts with a 4-byte boundary (1190+30+15+1=1236, 27486
> +30+17+3=27536, etc). This is, probably, allows the use of mmap for the
> resources.

So?  Someone may be using the extra fields to pad things, but for the purpose
of this issue that is completely irrelevant.  We only care about the
proper structure
of the file.  Besides, without clear reference to source code or a
specification any
hypothesis of padding is hearsay.

Did you look at the decoding I sent?  The extra length field length is clearly
reported as a size of one and the contents of the extra field are set to '\x00'.
The extra field of size one is the actual problem, not padding.

>> As Martin pointed out, the standard says that things must be in
>> multiples of 4-bytes.
>
> More precisely, the extra field must have at least 4-bytes length to fit
> a header. The standard is insufficiently defined in terms of what would
> happen if the rest of the field is less than 4 bytes (this is hidden
> behind by ellipsis).

How is it insufficiently defined at all?  It says [1]:

          In order to allow different programs and different types
          of information to be stored in the 'extra' field in .ZIP
          files, the following structure should be used for all
          programs storing data in this field:

          header1+data1 + header2+data2 . . .

          Each header should consist of:

            Header ID - 2 bytes
            Data Size - 2 bytes

          Note: all fields stored in Intel low-byte/high-byte order.

The ellipsis is just a standard convention for indicating a repeating
pattern.  Extra fields which are not multiples of four bytes are not
properly formed.

>>   So the record is non-portable.
>
> De jure the record is non-portable. De facto the record is portable
> (many other tools supports it). But even if it does not portable, we are
> dealing with the expansion of the zip format, which is very easy support
> for reading.

Like I said before, I am all for dropping extra fields we can not
interpret.  However,
let us be clear that with respect to the standard we are implementing
that zip files
constructed like this are ill-formed.

[1] http://www.pkware.com/documents/casestudies/APPNOTE.TXT

> This is definitely a padding issue. All uncompressed files are located
> so that the data starts with a 4-byte boundary (1190+30+15+1=1236, 27486
> +30+17+3=27536, etc). This is, probably, allows the use of mmap for the
> resources.

That can't possibly be the reason. mmap requires 4k (4096) alignment (on 
x86; more than that on SPARC).

Perhaps there is a reason for 4-alignment - but it can't be mmap-

> Like I said before, I am all for dropping extra fields we can not
> interpret.  However,
> let us be clear that with respect to the standard we are implementing
> that zip files > constructed like this are ill-formed.

I would still like to see the zipfile module to be able to detect 
ill-formed zipfiles - certainly not by default. This can be useful
to check zipfiles that somebody produces. As the Python check wouldn't
be complete, one would certainly need to apply other checks as well.

> That can't possibly be the reason. mmap requires 4k (4096) alignment (on 
> x86; more than that on SPARC).

This may be the reason to mmap the entire file and then read aligned
binary data.

> I would still like to see the zipfile module to be able to detect 
> ill-formed zipfiles - certainly not by default.

Agreed.  We already do produce some informational messages via the 
documented 'ZipFile.debug' attribute.  Just checking that flag and
producing a message to stdout like we do in other cases is one option.
Another, as you suggested already, is to provide a strict flag
that indicates an exception should be thrown upon encountering an 
ill-formed file.

I like the idea of reusing what we already have.  Although, when it 
fails, it doesn't fail loudly.

Any chance to commit the patch before final feature freeze?

Strictly speaking, the feature freeze has passed already, so you'll need the time machine.

However, this is not the *final* feature freeze - there will be a 3.4 release of Python also.

This issue is currently marked as a bugfix for current releases also. Since the proposal seems to be to introduce a new flag, should it be re-classified as enhancement (for 3.4)?

I think this is a bugfix. I also don't think that "strict" attribute
makes sense if we can't guarantee strong validity (see issue15114).

I still don't see the bug; the module is behaving correctly - it is the zipfile that is buggy. Supporting this specific kind of buggy zipfiles is a new feature.

For reasons discussed, I also disagree with the proposed patch (file24902). If this helps to advance this issue, I can formally reject it if desired.

Given that the overall trend in the stdlib seems to be to be more lenient in parsing, as with htmlparser, and that the not-really-strict 'strict' parameter for that is being removed, (#15114) I do not understand the objection to accepting one more malformation in zip files and the desire to add a not-really-strict 'strict' parameter here. Perhaps this philosophical issue should be discussed on pydev.

> I still don't see the bug; the module is behaving correctly - it is the zipfile that is buggy. Supporting this specific kind of buggy zipfiles is a new feature.

I agree. The module is behaving correctly.

> FWIW, I think it will OK to just ignore extra fields that we can't 
> interpret as according to the standard.  The only one we currently
> care about is the "Zip64 extended information extra field".  Also,
> other programs (including the Info-Zip tools) seem to mostly ignore
> these fields.

Yes, please.

> The ellipsis is just a standard convention for indicating a repeating
> pattern.  Extra fields which are not multiples of four bytes are not
> properly formed.

. . .

[;pause;]

Totally agree.  But at the very least it should check the size and raise a proper exception (e.g. BadZipFile).

FWIW, the code is already avoiding proper bounds checking using the built-in behavior of slicing -- 

    tp, ln = unpack('<HH', extra[:4])
    ...
    extra = extra[ln+4:]

-- which would otherwise throw exceptions if slice didn't innately and silently fail (due to an index being out-of-bounds).


> I still don't see the bug; the module is behaving correctly - it is the zipfile that is buggy. Supporting this specific kind of buggy zipfiles is a new feature.

You realize that you are calling user-controlled data buggy, right?

New changeset 6dd5e9556a60 by Gregory P. Smith in branch '2.7':
Fix issue #14315: The zipfile module now ignores extra fields in the central
http://hg.python.org/cpython/rev/6dd5e9556a60

New changeset 33843896ce4e by Gregory P. Smith in branch '3.4':
Fix issue #14315: The zipfile module now ignores extra fields in the central
http://hg.python.org/cpython/rev/33843896ce4e

New changeset 89be1419472c by Gregory P. Smith in branch 'default':
Fix issue #14315: The zipfile module now ignores extra fields in the central
http://hg.python.org/cpython/rev/89be1419472c

This was never an enhancement.  zipfile was failing to properly deal with real world data that other zip file tools on the planet were perfectly happy to deal with.  That's a bug.  Fixed.

 Practicality beats purity.
 Be lenient in what you accept.

The zipfile module is not meant to be a zip "standard" validation tool.

The other discussions in this bug about adding actual features such as a "strict" mode flag could be done but should really go in feature requests of their own.  The zipfile module is... not a wonderful body of code (understatement).  Example: It is still quite possible to get non zipfile.BadZipFile exceptions such as struct.error based on various arrangements of input data.

Just for information: the padding in APK files is added by the zipalign utility [1].

[1] https://developer.android.com/studio/command-line/zipalign