If you are

1) using zipimport
2) zipimport.c has cached the zip file's central index during an import.
3) the .zip file is modified or replaced while the process is running
4) you try to import something new from that .zip file.

you're gonna have a bad time... Typically a ZipImportError or some other form of ImportError.

What happen's is that Modules/zipimport.c is caching the end-of-zipfile central index for the .zip file and reusing that data on subsequent imports from that .zip without either (a) keeping the file handle open or (b) confirming that the .zip file has not changed.

I doubt many users are running into this.  But one situation where you do is when you zip up the Python standard library for your installed python and a long running process encounters data using a different encoding which triggers an import of an encodings.foo module after another process has come along and upgraded the standard library .zip file as part of updating your python installation...

I've got a fix in the works.  Test attached.

For the fix I am going with option (b) to reconfirm the validity of the .zip file any time something is imported from it rather than option (a) as leaving a new file handle open for the duration of the process, while the _correct ideal design_ seems intrusive for stable bug fix release.

I have only confirmed the bug on Python 2.7 & 3.3; i'll test default (3.4) after working on those two.

Here's a fix (the test is now in the patch).

attaching a patch for 3.3.

up next, 3.4: So long as I get to it before the release candidates the approach can likely be improved for 3.4 to actually hold the zip file we're importing from open for the life of the process instead of doing all of these stat calls.  That'd also get rid of the platform specific trick used to get the os.fstat function without being able to import the os module.

New changeset 8dbf8edb7128 by Gregory P. Smith in branch '2.7':
Fixes issue19081: When a zipimport .zip file in sys.path being imported
http://hg.python.org/cpython/rev/8dbf8edb7128

New changeset 90a99059aa36 by Gregory P. Smith in branch '2.7':
news entry for issue19081 fix.
http://hg.python.org/cpython/rev/90a99059aa36

New changeset cbeb22969da1 by Gregory P. Smith in branch '2.7':
normalize whitespace from prior issue19081 fix commit.
http://hg.python.org/cpython/rev/cbeb22969da1

The Windows 7 buildbot is unhappy after that change and failing one of the new tests:

ERROR: testZipFileChangesAfterFirstImport (test.test_zipimport.ZipFileModifiedAfterImportTestCase)
Alter the zip file after caching its index and try an import.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\buildbot.python.org\2.7.kloth-win64\build\lib\test\test_zipimport.py", line 446, in testZipFileChangesAfterFirstImport
    ziptest_a = __import__("ziptest_a", globals(), locals(), ["test_value"])
ZipImportError: bad local file header in C:\buildbot.python.org\2.7.kloth-win64\build\build\test_python_4388\junk95142.zip


It sounds like it was unable to find or properly use an fstat function. I'll fix it.

New changeset e09644eb6b20 by Gregory P. Smith in branch '2.7':
Should fix the issue19081 fix on Windows.  Don't let the previous
http://hg.python.org/cpython/rev/e09644eb6b20

updated 3.3 patch based off the changes made to the 2.7 one.

New changeset 2807a5f011e4 by Gregory P. Smith in branch '3.3':
Fixes Issue #19081: When a zipimport .zip file in sys.path being imported from
http://hg.python.org/cpython/rev/2807a5f011e4

New changeset 20b77ff040b6 by Gregory P. Smith in branch 'default':
Fixes issue #19081: When a zipimport .zip file in sys.path being imported from
http://hg.python.org/cpython/rev/20b77ff040b6

New changeset 5609135c6e86 by Gregory P. Smith in branch '2.7':
cleanup for the issue 19081 fix - pull the file open and close outside of the
http://hg.python.org/cpython/rev/5609135c6e86

I believe this is done.  I'm opting not to go for a more complicated "cache the open FILE* with the zip_directory_cache" approach for 3.4 due to complexity and time.

long term: It'd be ideal if zipimport weren't a pile of C code separate from the zipfile module itself.  importlib is baked in, perhaps a pure python zipimport and zipfile could be baked in someday.

While replacing zipimport with a pure Python version using importlib would be a great feather to have in importlib's cap, the dependencies in zipfile would make that somewhat difficult: http://hg.python.org/cpython/file/f1f707dd7cae/Lib/zipfile.py

Not all cases of this were fixed by the existing patch.  subimports still trigger the bug via a different code path.  attaching an updated unittest that demonstrates that.

The call stack for that code path failing is:

#0  get_data (fp=0xf64920, archive=0xe77ecc "/home/greg/sandbox/python/cpython/2.7/junk95142.zip", 
    toc_entry=('/home/greg/sandbox/python/cpython/2.7/junk95142.zip/ziptestpackage/ziptestmodule.py', 0, 16, 16, 0, 344, 17462, -1268077639)) at ./Modules/zipimport.c:1038
#1  0x00000000005401b3 in get_code_from_data (archive=0xe77ecc "/home/greg/sandbox/python/cpython/2.7/junk95142.zip", 
    fp=0xf64920, ispackage=0, isbytecode=0, mtime=0, 
    toc_entry=('/home/greg/sandbox/python/cpython/2.7/junk95142.zip/ziptestpackage/ziptestmodule.py', 0, 16, 16, 0, 344, 17462, -1268077639)) at ./Modules/zipimport.c:1267
#2  0x00000000005404a9 in get_module_code (self=0xe6b6f0, fullname=0xe6f0ec "ziptestpackage.ziptestmodule", 
    p_ispackage=0x7fffffff98d8, p_modpath=0x7fffffff98e8) at ./Modules/zipimport.c:1325
#3  0x000000000053dd77 in zipimporter_load_module (obj=<zipimport.zipimporter at remote 0xe6b6f0>, 
    args=('ziptestpackage.ziptestmodule',)) at ./Modules/zipimport.c:352
#4  0x00000000005646b8 in PyCFunction_Call (
    func=<built-in method load_module of zipimport.zipimporter object at remote 0xe6b6f0>, 
    arg=('ziptestpackage.ziptestmodule',), kw=0x0) at Objects/methodobject.c:81
#5  0x0000000000420f97 in PyObject_Call (
    func=<built-in method load_module of zipimport.zipimporter object at remote 0xe6b6f0>, 
    arg=('ziptestpackage.ziptestmodule',), kw=0x0) at Objects/abstract.c:2529
#6  0x00000000004210ed in call_function_tail (
    callable=<built-in method load_module of zipimport.zipimporter object at remote 0xe6b6f0>, 
    args=('ziptestpackage.ziptestmodule',)) at Objects/abstract.c:2561
#7  0x00000000004214f2 in PyObject_CallMethod (o=<zipimport.zipimporter at remote 0xe6b6f0>, 
    name=0x5a6c7d "load_module", format=0x5a6b67 "s") at Objects/abstract.c:2638
#8  0x00000000004f3aa0 in load_module (name=0xf67630 "ziptestpackage.ziptestmodule", fp=0x0, 
    pathname=0xf68660 "/home/greg/sandbox/python/cpython/2.7/junk95142.zip/ziptestpackage", type=9, 
    loader=<zipimport.zipimporter at remote 0xe6b6f0>) at Python/import.c:1961
#9  0x00000000004f5a67 in import_submodule (mod=<module at remote 0xe6c8e8>, subname=0xf6763f "ziptestmodule", 
    fullname=0xf67630 "ziptestpackage.ziptestmodule") at Python/import.c:2700
#10 0x00000000004f5037 in load_next (mod=<module at remote 0xe6c8e8>, altmod=<module at remote 0xe6c8e8>, 
    p_name=0x7fffffff9c88, buf=0xf67630 "ziptestpackage.ziptestmodule", p_buflen=0x7fffffff9ca0)
    at Python/import.c:2515

The problem appears to be that every zipimporter instance keeps its own reference to the zip files table of contents (self->files) instead of continually using the module wide zip_directory_cache but the zip_stat_cache is being maintained externally to that in a module wide fashion.

"import foo.bar" causes an "import foo" followed by an "import foo.bar" which uses a different zipimporter instance.  if it already exists, it's own self->files reference will be out of date despite the module global zip_stat_cache having been updated to say the TOC has been reread.

One solution to this would be to get rid of struct _zipimporter.files entirely and always use zip_directory_cache[self->archive] as the canonical single source for that.

"pro-tip" for anyone working on Python importers: You don't know how import works. Multiple instances of your importers will exist. If you think you know how import works, see the first statement again.

Here's a fix that I believe works with one TODO in there that needs investigation due to a question about the current historical zipimport code.

Can't you at least say "you don't know how import works unless you're Canadian"? =)

I dunno Brett - have you read the extension module import code and the zipimport code lately? I don't think I'll be willing to claim to fully understand even the default importers until we've rewritten those as PEP 451 compliant Python code with a couple of essential C level helper functions, just like the rest of the import system :)

As far as this particular issue goes, removing the extra level of caching sounds like a good idea, and http://bugs.python.org/file33614/issue19081-subimport-fix-gps02.diff looks right to me.

I refactored the unittests a bit and added another test for subimports when a directory within the .zip file is in sys.path as well.

The quizzical "wtf" TODO I had in the code has been "answered" in that I am unable to trigger a situation where path != buf and prefix is nonempty in that function.  I'm leaving that as a NOTE in the code for future investigation, but it isnt' related to this bug.  I cleaned up the code there regardless to refer to buf (guaranteed to be the path to the actual .zip file) rather than an odd mixture of path and buf.

Updated to make that function (zipimporter_init) easier to follow by not repurposing the path variable mid-function and allocate the local path buffer rather than keeping it on the stack.

New changeset 323cb02abfc6 by Gregory P. Smith in branch '2.7':
Issue #19081: Remove the zipimporter.files reference as the zip TOC
http://hg.python.org/cpython/rev/323cb02abfc6

Just to answer Nick's question: I read the extension import code just last month to see if I could update it for PEP 451 fast enough (obviously the answer was no =). As for zipimport, I read that (and the extension import code yet again) in the 3.3 timeframe (and did not find the experience pleasant), so I don't know if that's too far in the past for you. =)

Heh, OK. I've managed to avoid learning the gory details of the zipimporter
internals so far, and the details of Windows DLL loading is the gap on the
extension module side :)

New changeset d140e3f41796 by Gregory P. Smith in branch '2.7':
Refactor the new test for issue19081 to exec import statements into a
http://hg.python.org/cpython/rev/d140e3f41796

Fixed in the 2.7 branch.  The existing patch in the 3.3 and 3.4 branches is incomplete and does not actually fix the problem.  Despite what the Misc/NEWS entry claims.  The patch I am attaching now (applies to 3.3, I will forward port it to 3.4) fixes the remaining issue with zipimport failing subimports when the zip file has changed while the process is running.

Marking as a release blocker as one of the following needs to happen for both 3.3 and 3.4 prior to their final release:

A) The -gps05 patch needs to be applied (and forward ported to 3.4); I can do that.

B) The Misc/NEWS entry claiming that this issue is fixed should simply be removed from the Misc/NEWS file and the releases should happen without this patch.  After 3.4.0 and 3.3.4 I will commit this patch and re-add the Misc/NEWS entry under the 3.4.1 and 3.3.5 sections.

Release managers for 3.3 and 3.4, please chime in. (+nosy'd)

New changeset ca5431a434d6 by Gregory P. Smith in branch '2.7':
Remove unneeded use of globals() and locals() in test on imports
http://hg.python.org/cpython/rev/ca5431a434d6

Since this is a pretty big code churn, I'd prefer B) for 3.3.4.  (3.3.5 will be soon anyway.)

Thanks Georg.  I'll leave it until after the 3.3.4 release.  For simplicity's sake I'll leave it for 3.4.1 as well unless Larry says otherwise.

New changeset ecb272bae417 by Gregory P. Smith in branch '3.3':
Remove issue 19081 Misc/NEWS entry since it isn't entirely fixed yet.
http://hg.python.org/cpython/rev/ecb272bae417

New changeset 03fc7449f204 by Gregory P. Smith in branch 'default':
Remove issue 19081 Misc/NEWS entry since it isn't entirely fixed yet.
http://hg.python.org/cpython/rev/03fc7449f204

New changeset 3dd8b0d31543 by Benjamin Peterson in branch '2.7':
backout #19081 to fix #20621
http://hg.python.org/cpython/rev/3dd8b0d31543

Note everything was backed out for #19081.

New changeset 10f09881320d by Benjamin Peterson in branch '2.7':
finish backing out #19081
http://hg.python.org/cpython/rev/10f09881320d

New changeset 3350c6b7aa28 by Benjamin Peterson in branch '2.7':
remove tests for #19081
http://hg.python.org/cpython/rev/3350c6b7aa28

What do you mean by "everything"? How much did you back out?  I *ONLY* wanted the patches I posted in 20621 backed out as those were the source of the problem.

The changes made on 2014-01-06 and 2014-01-07 were good even though they only addressed part of the problem (as identified in my 2014-01-21 comment).

Also, as there is no 2.7 release imminent I wish you would've left the problem alone in 2.7 for me to FIX the problem instead of having to reapply the whole set.  Too late now. :(

In 2.7, I backed out 8dbf8edb7128 - f9c54ada1b32 in zipimport. Most of the commits mentioned this issue, so I assumed they were related. I apologize if that was too much. It was quite messy with all the subsequent fixups with the original commit.

Anyway, isn't the change 20b77ff040b6 on Jan 07 exactly what is backed out in the patch on #20621?

On 3.3/default, d28242a636c7, 2807a5f011e4, fafac90b69c4 were removed.

At this point i'll be reapplying things since 8dbf8edb7128 to 2.7 and trying to untangle where the problem came in.  I confirmed with diff that you've backed everything out to the pre-January state in all branches.

I've had part of those 2.7 patches deployed on 100,000 machines for many months with a zipped up standard library though the remaining bug causing the fix to not work in several situations mentioned in http://bugs.python.org/issue19081#msg208754 is still present in what i'm running.

Thanks for the rollbacks to unblock 3.3 and 3.4!

I'm unassigning as i won't be figuring out how to hackishly fix this again.  The best solution is a complete rewrite of zipimport.

this remains a known issue: if you use zipimport, do not allow the zip file to change out from underneath your running process or you will have "fun".

zipimport have been rewritten in pure Python (issue25711).

The issue still remains in Python 3.8.

and I got ZipImportError: bad local file header

In playing with Lib/zipfile.py and Lib/zipimport.py, I noticed that zipfile has supported opportunistic loading of bz2/lzma for ~9 years. However, zipimport assumes only zlib will be used. (Yet, zipfile.PyZipFile will happily create zlib/bz2/lzma ZIP archives ... zipfile.PyZipFile('mod.zip', 'w', compression=zipfile.ZIP_LZMA) for example.)

At first I wondered why zipimport essentially duplicates a lot from zipfile but then realized (after reading some of the commit messages around the pure-python rewrite of zipimport a few years ago) that since zipimport is called as part of startup, there's a need to avoid importing certain modules. 

I'm wondering if this specific issue with zipimport is possibly more of an indicator of a larger issue? 

Specifically:

* The duplication of code between zipfile and zipimport seems like a potential source of bugs - I get the rationale but perhaps the "base" ZIP functionality ought to be refactored out of both zipimport and zipfile so they can share... And I mean the low-level stuff (compressor, checksum, etc.). Zipfile definitely imports more than zipimport but I haven't looked at what those imports are doing extensively. 

Ultimately: the behavior of the new ZipImport appears to be, essentially, the same as zipimport.c:

Per PEP-302 [https://www.python.org/dev/peps/pep-0302/], zipimport.zipimporter gets registered into sys.path_hooks. When you import anything in a zip file, all of the paths get cached into sys.path_importer_cache as zipimport.zipimporter objects.

The zipimporter objects, when instantiated, run zipimport._read_directory() which returns a low level dict with each key being a filename (module) and each value being a tuple of low-level metadata about that file including the byte offset into the zip file, time last modified, CRC, etc. (See zipimport.py:330 or so). This is then stored in zipimporter._files.

Critically, the contents of the zip file are not decompressed at this stage: only the metadata of what is in the zip file and (most importantly) where it is, is stored in memory: only when a module is actually called for loading is the data read utilizing the cached metadata. There appears to be no provision for (a) verifying that the zip file itself hasn't changed or (b) refreshing the metadata. So it's no surprise really that this error is happening: the cached file contents metadata instructs zipimporter to decompress a specific byte offset in the zip file *when an import is called*. If the zip file changes on disk between the metadata scan (e.g. first read of the zip file) and actual loading, bam: error.

There appear to be several ways to fix this ... I'm not sure of the best:

* Possibly lock the ZIP file on first import so it doesn't change (this presents many new issues)
* Rescan the ZIP before each import; but the point of caching the contents appears to be the avoidance of this
* Hash the entire file and compare (expensive CPU-wise)
* Rely on modified time? e.g. cache the whole zip modified time at read and then if that's not the same invalidate the cache and rescan
* Cache the entire zip file into memory at first load - this has some advantages (can store the ZIP data compressed; would make the import all or nothing; faster?) But then there would need to be some kind of variable to limit the size/total size - it becomes a memory hog...

On POSIX systems, keeping the file open means you will keep a handle to the original file in the case where something moves a new file into it's place (as is normal during software package updates) or otherwise unlinks the original.  That is the situation that led to filing this issue and is one that is technically solvable by keeping the file handle open and always using that for access.

We can't do anything very meaningful about someone opening the existing file in 'w+a' mode and scribbling other bytes over it.  I wouldn't try to protect against that.  "locking" a file isn't an option on most platforms and when available, is very unusual to do in this century.