ZipFile.open() should not reopen the underlying file

[kasal]

BPO	14099
Nosy	@loewis, @pitrou, @larryhastings, @benjaminp, @mcepl, @merwok, @bitdancer, @dw, @serhiy-storchaka, @eryksun
Files	Proposed-fix-of-issue14099.patch: proposed fix, against latest source tree Proposed-fix-of-issue14099-second.patch: Updated patch zipfile_concurrent_read_1.diff: Draft patch (from dw) against 524a004e93dd zipfile_share_file.patch bench_zip.py zipfile_tellable.patch: Support tellable but unseekable files in ZipFile.writestr() zipfile_threadsafe.patch: Makes ZipFile threadsafe

^{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 2015-01-26.12:05:57.524>
created_at = <Date 2012-02-23.10:46:31.740>
labels = ['type-bug', 'library']
title = 'ZipFile.open() should not reopen the underlying file'
updated_at = <Date 2015-02-05.21:24:35.655>
user = 'https://bugs.python.org/kasal'

bugs.python.org fields:

activity = <Date 2015-02-05.21:24:35.655>
actor = 'eryksun'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2015-01-26.12:05:57.524>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2012-02-23.10:46:31.740>
creator = 'kasal'
dependencies = []
files = ['24617', '24624', '37191', '37197', '37198', '37732', '37733']
hgrepos = []
issue_num = 14099
keywords = ['patch']
message_count = 30.0
messages = ['154058', '154077', '154110', '154123', '176844', '176853', '176855', '176857', '176858', '176859', '231132', '231181', '231191', '231473', '231475', '231918', '232032', '232035', '232048', '232055', '232068', '232070', '233553', '234142', '234143', '234146', '234735', '234736', '235183', '235453']
nosy_count = 17.0
nosy_names = ['loewis', 'alanmcintyre', 'pitrou', 'ocean-city', 'larry', 'benjamin.peterson', 'mcepl', 'durin42', 'eric.araujo', 'Arfrever', 'r.david.murray', 'python-dev', 'Matt.Mackall', 'kasal', 'dw', 'serhiy.storchaka', 'eryksun']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue14099'
versions = ['Python 3.5']

[kasal]

When a file inside a zip is open, the underlying zip file is open again.
(Unless the file name is unknown, because the ZipFile object was created with fp only.)

This design is incorrect, insecure, and ineffective:

• the reopen uses the same string as file name, but on unix-like systems that file name may no longer exist, or may point to a different file
• opening n files from the same zip archive consumes n OS file descriptors, wasting resources

I believe that the parent ZipFile object and all the child ZipExtFile objects should keep the same fp. The last one would close it.

I'm working on a patch currently.

[kasal]

Attached please find a patch that fixes this issue by reusing the original fp from ZipFile object.

Two of the test cases attempted to read a file from a zip as soon as write() was called. I believe that this is not correct usage: zip file is not even fully written to disk at that stage!
So I took the liberty to change these two test cases so that they first write the file and then read it.

Let me thank to Martin Sikora for discovering the issue and to Matej Cepl for testing it on current source tree.

[merwok]

Thanks for the report and patch. I’m afraid changing the constructor signature is not an option, due to our backward compatibility policy. Do you think the bug can be fixed without changing the signature, or with new arguments added after the existing ones?

[kasal]

Attached please find a second iteration of the fix.
This time the signature of ZipExtFile is kept backward compatible, with one new parameter added.

[serhiy-storchaka]

This issue looks as a duplicate or a superseder of bpo-10631. See also bpo-16569.

seek() for every read should significantly decrease performance. It may be worth to prohibit the simultaneous reading of different files from the archive. In any case the children counting in the patch looks doubtful.

[kasal]

Re: children counting

You need to know the number of open children and whether the parent ZipFile object is still open.
As soon as both all children and the parent ZipFile are closed, the underlying fp (corresponding to the file name given initially) shall be closed as well.

The code submitted in the patch ensures that. But other implementations are possible.

In any case, it is necessary to ensure that the children stay usable even if the parent ZipFile is closed, because of code like this:

    def datafile(self):
        with ZipFile(self.datafilezip, "r") as f:
            return f.open("data.txt")

This idiom currently works and should not be broken.

Re: seek()

The read can interfere not only with a parallel file expansion, but also with a ZipFile metadata read (user can list the contents of the zip again). Both of these would have to be forbidden by the documentation, and, ideally, also enforced. (As disscussed issue bpo-16569)

OTOH, zipfile.py is already slow, because the decompression is implemented in Python as interpreted code. I guess that the slowdown by seek() is neglectable compared to this.
Also note that we most often seek to the current position; the OS should notice that and return swiftly.

[serhiy-storchaka]

I think some benchmarks will needed to see how it will affect the performance.

Please update your patch to current sources. The module code was changed last months.

[kasal]

I'm not sure when I'll get to this, sorry.
Hopefully sometime soon.

[serhiy-storchaka]

This idiom currently works and should not be broken.

Hmm. This seems doubtful to me, but if it is used, then I agree, it shouldn't be broken.

I guess that the slowdown by seek() is neglectable compared to this.

Even one function call can have effect on performance of short reads (bpo-10376, bpo-16304). Fortunately in this corner case the read buffer will be used.

Also note that we most often seek to the current position; the OS should notice that and return swiftly.

This may affect the buffered Python file (I did not check). The OS also doesn't notice this if the OS is Windows (bpo-8745).

I want to see and test an updated patch.

[serhiy-storchaka]

Sorry, not bpo-16304, but bpo-16034. The commit messages were wrong.

[dw]

Per my comment on bpo-16569, the overhead of performing one seek before each (raw file data) read is quite minimal. I have attached a new (but incomplete) patch, on which the following microbenchmarks are based. The patch is essentially identical to Stepan's 2012 patch, except I haven't yet decided how best to preserve the semantics of ZipFile.close().

"my.zip" is the same my.zip from bpo-22842. It contains 10,000 files each containing 10 bytes over 2 lines.

"my2.zip" contains 8,000 files each containing the same copy of 64kb of /dev/urandom output. The resulting ZIP is 500mb.

For each test, the first run is the existing zipfile module, and the second run is with the patch. In summary:

• There is a 35% perf increase in str mode when handling many small files (on OS X at least)
• There is a 6% perf decrease in file mode when handling small sequential reads.
• There is a 2.4% perf decrease in file mode when handling large sequential reads.

From my reading of zipfile.py, it is clear there are _many_ ways to improve its performance (probably starting with readline()), and rejection of a functional fix should almost certainly be at the bottom of that list.

For each of the tests below, the functions used were:

    def a():
        """
        Test concurrent line reads to a str mode ZipFile.
        """
        zf = zipfile.ZipFile('my2.zip')
        members = [zf.open(n) for n in zf.namelist()]
        for m in members:
            m.readline()
        for m in members:
            m.readline()

    def c():
        """
        Test sequential small reads to a str mode ZipFile.
        """
        zf = zipfile.ZipFile('my2.zip')
        for name in zf.namelist():
            with zf.open(name) as zfp:
                zfp.read(1000)

    def d():
        """
        Test sequential small reads to a file mode ZipFile.
        """
        fp = open('my2.zip', 'rb')
        zf = zipfile.ZipFile(fp)
        for name in zf.namelist():
            with zf.open(name) as zfp:
                zfp.read(1000)

    def e():
        """
        Test sequential large reads to a file mode ZipFile.
        """
        fp = open('my2.zip', 'rb')
        zf = zipfile.ZipFile(fp)
        for name in zf.namelist():
            with zf.open(name) as zfp:
                zfp.read()

---- my.zip ----

$ python3.4 -m timeit -s 'import my' 'my.a()'
10 loops, best of 3: 1.47 sec per loop

$ python3.4 -m timeit -s 'import my' 'my.a()'
10 loops, best of 3: 950 msec per loop

---

$ python3.4 -m timeit -s 'import my' 'my.c()'
10 loops, best of 3: 1.3 sec per loop

$ python3.4 -m timeit -s 'import my' 'my.c()'
10 loops, best of 3: 865 msec per loop

---

$ python3.4 -m timeit -s 'import my' 'my.d()'
10 loops, best of 3: 800 msec per loop

$ python3.4 -m timeit -s 'import my' 'my.d()'
10 loops, best of 3: 851 msec per loop

---- my2.zip ----

$ python3.4 -m timeit -s 'import my' 'my.a()'
10 loops, best of 3: 1.46 sec per loop

$ python3.4 -m timeit -s 'import my' 'my.a()'
10 loops, best of 3: 1.16 sec per loop

---

$ python3.4 -m timeit -s 'import my' 'my.c()'
10 loops, best of 3: 1.13 sec per loop

$ python3.4 -m timeit -s 'import my' 'my.c()'
10 loops, best of 3: 892 msec per loop

---

$ python3.4 -m timeit -s 'import my' 'my.d()'
10 loops, best of 3: 842 msec per loop

$ python3.4 -m timeit -s 'import my' 'my.d()'
10 loops, best of 3: 882 msec per loop

---

$ python3.4 -m timeit -s 'import my' 'my.e()'
10 loops, best of 3: 1.65 sec per loop

$ python3.4 -m timeit -s 'import my' 'my.e()'
10 loops, best of 3: 1.69 sec per loop

[serhiy-storchaka]

Thank you David for your benchmarks and patch. There are several backward compatibility issues with the reading from ZipFile opened for write and from closed ZipFile. This behavior is mostly undocumented (except the reading from closed ZipFile), but even our tests depend on it and changing it could break user code with good chance.

Here is a patch which preserves current behavior. Added new tests to check this behavior explicitly. Other advantage of the patch is that it doesn't change the signature of ZipExtFile constructor at all.

Benchmarks don't show stable significant difference between patched and unpatched versions.

[dw]

Hi Serhiy,

Thanks for the new patch, it looks better than my attempt. :)

[serhiy-storchaka]

I hesitate about applying the patch to maintained releases. On one hand, besides interface (even non-documented details) left the same, the patch changes interiors too much for ordinal bug. I don't see how it can break something, but this doesn't guarantee that changes don't have unexpected effect.

On other hand, this bug can be considered as security-related issue. Malicious local attacker could replace ZIP file between its open and read from it or between two reads, if he has write access to the directory containing ZIP file or there are symplinks under his control in ZIP file path. The danger of this may exceed hypothetical negative consequences of the applying of the patch.

I appeal the matter to release managers. Should we apply this patch (the risk is pretty small) to 2.7 and 3.4?

[dw]

While in spirit this is a bug fix, it's reasonably complex and affects a popular module -- I'm not sure it should be applied to 2.x, and probably not in a minor release of 3.x either. Would it make sense to include as part of 3.5?

(That said, I'd love to see this fixed in 2.x ;))

[serhiy-storchaka]

What your thoughts Benjamin? Should this patch be applied to 2.7.10 (this is not critical for 2.7.9)?

[benjaminp]

Okay for 2.7.10.

[dw]

Could we also make a small tweak to zipfile.rst indicating the new behaviour? I had made an initial attempt in my patch but wasn't particularly happy with the wording.

[serhiy-storchaka]

How about just "Objects returned by :meth:`.open` can operate independently of the ZipFile."?

[dw]

Sounds great :)

[python-dev]

New changeset c2c4cde55f6f by Serhiy Storchaka in branch '2.7':
Issue bpo-14099: ZipFile.open() no longer reopen the underlying file. Objects
https://hg.python.org/cpython/rev/c2c4cde55f6f

New changeset e5bb3044402b by Serhiy Storchaka in branch '3.4':
Issue bpo-14099: ZipFile.open() no longer reopen the underlying file. Objects
https://hg.python.org/cpython/rev/e5bb3044402b

New changeset 334c01aa7f93 by Serhiy Storchaka in branch 'default':
Issue bpo-14099: ZipFile.open() no longer reopen the underlying file. Objects
https://hg.python.org/cpython/rev/334c01aa7f93

[serhiy-storchaka]

Thanks Stepan for the idea.

[MattMackall]

The committed fix breaks Mercurial.

http://bz.selenic.com/show_bug.cgi?id=4492

The "underlying file-like object" in our case is a wsgirequest but anything else trying to serve a dynamically-generated zip file on the web will probably die. We wrapped wsgirequest to support tell() many years ago probably copying someone else's hack, and it's worked fine across Python 2.4-2.7, but we fundamentally can't support all the new seek()s that were added here.

[serhiy-storchaka]

Thank you for your report Matt.

There is other problem. It is nowhere documented and newer granted and newer mentioned when ZipFile.open() was added, but file-like objects returned by ZipFile.open() could be read in different threads simultaneously. It makes sense because decompressors release GIL and parallel reading compressed file can has benefit.

It is easy to fix both issues (I prefer to do this in separate paths), but due to the overall complexity it is safer to withdraw committed changes in maintained releases and apply additional patches only in default branch.

[serhiy-storchaka]

Adding locks almost not affects performance, because reads are done by relative large chunks and locking overhead is small.

[serhiy-storchaka]

See also bpo-23252.

[python-dev]

New changeset ae42c4576438 by Serhiy Storchaka in branch '2.7':
Issue bpo-14099: Backout changeset c2c4cde55f6f (except adapted tests).
https://hg.python.org/cpython/rev/ae42c4576438

New changeset 680b47c96e08 by Serhiy Storchaka in branch '3.4':
Issue bpo-14099: Backout changeset e5bb3044402b (except adapted tests).
https://hg.python.org/cpython/rev/680b47c96e08

New changeset 4973ccd46e32 by Serhiy Storchaka in branch 'default':
Issue bpo-14099: Writing to ZipFile and reading multiple ZipExtFiles is
https://hg.python.org/cpython/rev/4973ccd46e32

New changeset 9cbf9f96920d by Serhiy Storchaka in branch 'default':
Issue bpo-14099: Restored support of writing ZIP files to tellable but
https://hg.python.org/cpython/rev/9cbf9f96920d

[serhiy-storchaka]

Sorry Stepan and David, but for this feature you need wait 3.5.

[python-dev]

New changeset 4f96e9a8eee8 by Serhiy Storchaka in branch 'default':
Don't seek to the start of the file when open ZipFile with the 'w' mode
https://hg.python.org/cpython/rev/4f96e9a8eee8

[eryksun]

The changeset from 03 Dec is in the Windows 2.7.9 release.

    Python 2.7.9 (default, Dec 10 2014, 12:28:03) [MSC v.1500 64 bit
    (AMD64)] on win32
    Type "help", "copyright", "credits" or "license" for more
    information.
    >>> import zipfile
    >>> zipfile._SharedFile
    <class zipfile._SharedFile at 0x0000000000F707C8>