GeezerVison is showing github.com — zipimport behaves badly when the zip file changes while the process is running · Issue #63281 · python/cpython
zipimport behaves badly when the zip file changes while the process is running #63281Description
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 = None
closed_at = None
created_at = <Date 2013-09-24.04:00:03.288>
labels = ['type-bug', '3.8']
title = 'zipimport behaves badly when the zip file changes while the process is running'
updated_at = <Date 2020-09-19.19:01:11.201>
user = 'https://github.com/gpshead' bugs.python.org fields:
activity = <Date 2020-09-19.19:01:11.201>
actor = 'georg.brandl'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = []
creation = <Date 2013-09-24.04:00:03.288>
creator = 'gregory.p.smith'
dependencies = []
files = ['31854', '31871', '33356', '33611', '33614', '33734', '33736', '33763']
hgrepos = []
issue_num = 19081
keywords = ['patch', 'needs review']
message_count = 43.0
messages = ['198351', '198363', '207419', '207458', '207462', '207514', '207519', '207655', '207656', '207657', '207689', '208754', '208756', '208761', '208769', '208825', '209148', '209350', '209375', '209414', '209444', '209479', '209504', '209508', '209510', '209529', '209575', '209707', '211344', '211347', '211349', '211350', '211351', '211352', '211353', '211354', '211356', '254708', '325722', '366658', '366688', '376862', '376884']
nosy_count = 14.0
nosy_names = ['gregory.p.smith', 'ncoghlan', 'larry', 'benjamin.peterson', 'stutzbach', 'Arfrever', 'flox', 'python-dev', 'eric.snow', 'serhiy.storchaka', 'superluser', 'iMath', 'tulir', 'danieljewell']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue19081'
versions = ['Python 3.8'] Activitygpshead commented on Sep 24, 2013 If you are
- using zipimport
- zipimport.c has cached the zip file's central index during an import.
- the .zip file is modified or replaced while the process is running
- 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. gpshead commented on Sep 24, 2013 Here's a fix (the test is now in the patch). gpshead commented on Jan 6, 2014 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. python-dev commented on Jan 6, 2014 gpshead commented on Jan 6, 2014 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.zipIt sounds like it was unable to find or properly use an fstat function. I'll fix it. python-dev commented on Jan 7, 2014 gpshead commented on Jan 7, 2014 updated 3.3 patch based off the changes made to the 2.7 one. python-dev commented on Jan 8, 2014 python-dev commented on Jan 8, 2014 gpshead commented on Jan 8, 2014 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. brettcannon commented on Jan 8, 2014 gpshead commented on Jan 22, 2014 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. gpshead commented on Feb 16, 2014 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! gpshead commented on Nov 16, 2015 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". serhiy-storchaka commented on Sep 19, 2018 zipimport have been rewritten in pure Python (bpo-25711). Reacted by Gregory P. Smith iMath commented on Apr 17, 2020 The issue still remains in Python 3.8. iMath commented on Apr 18, 2020 and I got ZipImportError: bad local file header danieldjewell commented on Sep 14, 2020 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...
gpshead commented on Sep 14, 2020 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. serhiy-storchaka commented on Jul 4, 2024 @gpshead, are you interested to convert your patches into a PR? zipimport was rewritten in Python, and some of your changes may no longer be actual. gpshead commented on Jul 4, 2024 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.
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|