zipimport behaves badly when the zip file changes while the process is running #63281
Comments
|
If you are
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': New changeset 90a99059aa36 by Gregory P. Smith in branch '2.7': New changeset cbeb22969da1 by Gregory P. Smith in branch '2.7': |
|
The Windows 7 buildbot is unhappy after that change and failing one of the new tests: ERROR: testZipFileChangesAfterFirstImport (test.test_zipimport.ZipFileModifiedAfterImportTestCase) 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.zipIt 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': |
|
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': New changeset 20b77ff040b6 by Gregory P. Smith in branch 'default': |
|
New changeset 5609135c6e86 by Gregory P. Smith in branch '2.7': |
|
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': |
|
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 |
|
New changeset d140e3f41796 by Gregory P. Smith in branch '2.7': |
|
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': |
|
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': New changeset 03fc7449f204 by Gregory P. Smith in branch 'default': |
|
New changeset 3dd8b0d31543 by Benjamin Peterson in branch '2.7': |
|
Note everything was backed out for bpo-19081. |
|
New changeset 10f09881320d by Benjamin Peterson in branch '2.7': |
|
New changeset 3350c6b7aa28 by Benjamin Peterson in branch '2.7': |
|
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 bpo-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 (bpo-25711). |
|
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:
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:
|
|
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. |
|
@gpshead, are you interested to convert your patches into a PR? |
|
The underlying issue remains so long as the zipimport code does not keep the zip file open for the life of the process. But agreed that any of my ancient patches are likely pointless now. I don't believe anyone is going to work on this. And with containers as the common deployment mechanism these days it should matter less. |
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: