diff -r f4377699fd47 Lib/test/test_zipimport.py --- a/Lib/test/test_zipimport.py Mon Jan 27 23:15:14 2014 +0200 +++ b/Lib/test/test_zipimport.py Tue Jan 28 00:15:55 2014 -0800 @@ -407,56 +407,143 @@ def setUp(self): zipimport._zip_directory_cache.clear() zipimport._zip_stat_cache.clear() + # save sys.modules so we can unimport everything done by our tests. + self._sys_modules_orig = dict(sys.modules) ImportHooksBaseTestCase.setUp(self) def tearDown(self): ImportHooksBaseTestCase.tearDown(self) + # The closest we can come to un-importing our zipped up test modules. + sys.modules.clear() + sys.modules.update(self._sys_modules_orig) if os.path.exists(TEMP_ZIP): os.remove(TEMP_ZIP) - def testZipFileChangesAfterFirstImport(self): - """Alter the zip file after caching its index and try an import.""" + def setUpZipFileModuleAndTestImports(self): + # Create a .zip file to test with + self.zipfile_path = TEMP_ZIP packdir = TESTPACK + os.sep files = {packdir + "__init__" + pyc_ext: (NOW, test_pyc), packdir + TESTMOD + ".py": (NOW, "test_value = 38\n"), "ziptest_a.py": (NOW, "test_value = 23\n"), "ziptest_b.py": (NOW, "test_value = 42\n"), "ziptest_c.py": (NOW, "test_value = 1337\n")} - zipfile_path = TEMP_ZIP - _write_zip_package(zipfile_path, files) - self.assertTrue(os.path.exists(zipfile_path)) - sys.path.insert(0, zipfile_path) + _write_zip_package(self.zipfile_path, files) + self.assertTrue(os.path.exists(self.zipfile_path)) + sys.path.insert(0, self.zipfile_path) + + self.testpack_testmod = TESTPACK + "." + TESTMOD + + with io.open(self.zipfile_path, "rb") as orig_zip_file: + self.orig_zip_file_contents = orig_zip_file.read() # Import something out of the zipfile and confirm it is correct. - testmod = __import__(TESTPACK + "." + TESTMOD, + testmod = __import__(self.testpack_testmod, globals(), locals(), ["__dummy__"]) self.assertEqual(testmod.test_value, 38) + del sys.modules[TESTPACK] + del sys.modules[self.testpack_testmod] + # Import something else out of the zipfile and confirm it is correct. ziptest_b = __import__("ziptest_b", globals(), locals(), ["test_value"]) self.assertEqual(ziptest_b.test_value, 42) + del sys.modules["ziptest_b"] - # Truncate and fill the zip file with non-zip garbage. - with open(zipfile_path, "rb") as orig_zip_file: - orig_zip_file_contents = orig_zip_file.read() - with open(zipfile_path, "wb") as byebye_valid_zip_file: + def truncateAndFillZipWithNonZipGarbage(self): + with io.open(self.zipfile_path, "wb") as byebye_valid_zip_file: byebye_valid_zip_file.write(b"Tear down this wall!\n"*1987) + + def restoreZipFileWithDifferentHeaderOffsets(self): + """Make it a valid zipfile with some garbage at the start.""" + # This alters all of the caches offsets within the file. + with io.open(self.zipfile_path, "wb") as new_zip_file: + new_zip_file.write(b"X"*1991) # The year Python was created. + new_zip_file.write(self.orig_zip_file_contents) + + def testZipFileChangesAfterFirstImport(self): + """Alter the zip file after caching its index and try an import.""" + self.setUpZipFileModuleAndTestImports() + # The above call cached the .zip table of contents during its tests. + self.truncateAndFillZipWithNonZipGarbage() # Now that the zipfile has been replaced, import something else from it # which should fail as the file contents are now garbage. with self.assertRaises(ImportError): - ziptest_a = __import__("ziptest_a", globals(), locals(), - ["test_value"]) - self.assertEqual(ziptest_a.test_value, 23) + ziptest_a = __import__("ziptest_a", {}, {}, ["test_value"]) + # The code path used by the __import__ call is different than + # that used by import statements. Try these as well. Some of + # these may create new zipimporter instances. We need to + # function properly using the global zipimport caches + # regardless of how many zipimporter instances for the same + # .zip file exist. + with self.assertRaises(ImportError): + import ziptest_a + with self.assertRaises(ImportError): + from ziptest_a import test_value + with self.assertRaises(ImportError): + exec("from {} import {}".format(TESTPACK, TESTMOD), {}) - # Now lets make it a valid zipfile that has some garbage at the start. - # This alters all of the offsets within the file - with open(zipfile_path, "wb") as new_zip_file: - new_zip_file.write(b"X"*1991) # The year Python was created. - new_zip_file.write(orig_zip_file_contents) + # Alters all of the offsets within the file + self.restoreZipFileWithDifferentHeaderOffsets() # Now that the zip file has been "restored" to a valid but different - # zipfile the zipimporter should *successfully* re-read the new zip - # file's end of file central index and be able to import from it again. - ziptest_c = __import__("ziptest_c", globals(), locals(), ["test_value"]) + # zipfile all zipimporter instances should *successfully* re-read the + # new file's end of file central index and be able to import again. + + # Importing a submodule triggers a different import code path. + test_ns = {} + exec("import " + self.testpack_testmod, test_ns) + self.assertEqual(getattr(test_ns[TESTPACK], TESTMOD).test_value, 38) + test_ns = {} + exec("from {} import {}".format(TESTPACK, TESTMOD), test_ns) + self.assertEqual(test_ns[TESTMOD].test_value, 38) + + ziptest_c = __import__("ziptest_c", {}, {}, ["test_value"]) + self.assertEqual(ziptest_c.test_value, 1337) + + def testZipFileSubpackageImport(self): + """Import via multiple sys.path entries into parts of the zip.""" + self.setUpZipFileModuleAndTestImports() + # Put a subdirectory within the zip file into the import path. + sys.path.insert(0, self.zipfile_path + os.sep + TESTPACK) + + testmod = __import__(TESTMOD, {}, {}, ["test_value"]) + self.assertEqual(testmod.test_value, 38) + del sys.modules[TESTMOD] + test_ns = {} + exec("from {} import test_value".format(TESTMOD), test_ns) + self.assertEqual(test_ns["test_value"], 38) + del sys.modules[TESTMOD] + + # Confirm that imports from the top level of the zip file + # (already in sys.path from the setup call above) still work. + ziptest_a = __import__("ziptest_a", {}, {}, ["test_value"]) + self.assertEqual(ziptest_a.test_value, 23) + del sys.modules["ziptest_a"] + import ziptest_c + self.assertEqual(ziptest_c.test_value, 1337) + del sys.modules["ziptest_c"] + + self.truncateAndFillZipWithNonZipGarbage() + # Imports should now fail. + with self.assertRaises(ImportError): + testmod = __import__(TESTMOD, {}, {}, ["test_value"]) + with self.assertRaises(ImportError): + exec("from {} import test_value".format(TESTMOD), {}) + with self.assertRaises(ImportError): + import ziptest_a + + self.restoreZipFileWithDifferentHeaderOffsets() + # Imports should work again, the central directory TOC will be re-read. + testmod = __import__(TESTMOD, {}, {}, ["test_value"]) + self.assertEqual(testmod.test_value, 38) + del sys.modules[TESTMOD] + test_ns = {} + exec("from {} import test_value".format(TESTMOD), test_ns) + self.assertEqual(test_ns["test_value"], 38) + + ziptest_a = __import__("ziptest_a", {}, {}, ["test_value"]) + self.assertEqual(ziptest_a.test_value, 23) + import ziptest_c self.assertEqual(ziptest_c.test_value, 1337) diff -r f4377699fd47 Modules/zipimport.c --- a/Modules/zipimport.c Mon Jan 27 23:15:14 2014 +0200 +++ b/Modules/zipimport.c Tue Jan 28 00:15:55 2014 -0800 @@ -39,7 +39,6 @@ decoded from the filesystem encoding */ PyObject *prefix; /* file prefix: "a/sub/directory/", encoded to the filesystem encoding */ - PyObject *files; /* dict with file info {path: toc_entry} */ }; static PyObject *ZipImportError; @@ -168,9 +167,6 @@ } Py_XDECREF(zip_stat); } - else - Py_INCREF(files); - self->files = files; /* Transfer reference */ self->archive = filename; @@ -203,22 +199,11 @@ return -1; } -/* GC support. */ -static int -zipimporter_traverse(PyObject *obj, visitproc visit, void *arg) -{ - ZipImporter *self = (ZipImporter *)obj; - Py_VISIT(self->files); - return 0; -} - static void zipimporter_dealloc(ZipImporter *self) { - PyObject_GC_UnTrack(self); Py_XDECREF(self->archive); Py_XDECREF(self->prefix); - Py_XDECREF(self->files); Py_TYPE(self)->tp_free((PyObject *)self); } @@ -305,7 +290,7 @@ static int check_is_directory(ZipImporter *self, PyObject* prefix, PyObject *path) { - PyObject *dirpath; + PyObject *dirpath, *files; int res; /* See if this is a "directory". If so, it's eligible to be part @@ -314,8 +299,22 @@ dirpath = PyUnicode_FromFormat("%U%U%c", prefix, path, SEP); if (dirpath == NULL) return -1; + + files = PyDict_GetItem(zip_directory_cache, self->archive); + if (files == NULL) { + /* Some scoundrel has cleared zip_directory_cache out from + * beneath us. Try repopulating it once before giving up. */ + FILE *fp = safely_reopen_archive(self); + if (fp == NULL) + return -1; + fclose(fp); + files = PyDict_GetItem(zip_directory_cache, self->archive); + if (files == NULL) + return -1; + } + /* If dirpath is present in self->files, we have a directory. */ - res = PyDict_Contains(self->files, dirpath); + res = PyDict_Contains(files, dirpath); Py_DECREF(dirpath); return res; } @@ -327,6 +326,7 @@ PyObject *subname; PyObject *path, *fullpath, *item; struct st_zip_searchorder *zso; + PyObject *files; subname = get_subname(fullname); if (subname == NULL) @@ -337,13 +337,26 @@ if (path == NULL) return MI_ERROR; + files = PyDict_GetItem(zip_directory_cache, self->archive); + if (files == NULL) { + /* Some scoundrel has cleared zip_directory_cache out from + * beneath us. Try repopulating it once before giving up. */ + FILE *fp = safely_reopen_archive(self); + if (fp == NULL) + return MI_ERROR; + fclose(fp); + files = PyDict_GetItem(zip_directory_cache, self->archive); + if (files == NULL) + return MI_ERROR; + } + for (zso = zip_searchorder; *zso->suffix; zso++) { fullpath = PyUnicode_FromFormat("%U%s", path, zso->suffix); if (fullpath == NULL) { Py_DECREF(path); return MI_ERROR; } - item = PyDict_GetItem(self->files, fullpath); + item = PyDict_GetItem(files, fullpath); Py_DECREF(fullpath); if (item != NULL) { Py_DECREF(path); @@ -592,7 +605,7 @@ #ifdef ALTSEP _Py_IDENTIFIER(replace); #endif - PyObject *toc_entry, *data; + PyObject *toc_entry, *data, *files; Py_ssize_t path_start, path_len, len; if (!PyArg_ParseTuple(args, "U:zipimporter.get_data", &path)) @@ -625,7 +638,13 @@ if (fp == NULL) goto error; - toc_entry = PyDict_GetItem(self->files, key); + files = PyDict_GetItem(zip_directory_cache, self->archive); + if (files == NULL) { + /* This should never happen as safely_reopen_archive() should + * have repopulated zip_directory_cache if needed. */ + goto error; + } + toc_entry = PyDict_GetItem(files, key); if (toc_entry == NULL) { PyErr_SetFromErrnoWithFilenameObject(PyExc_IOError, key); Py_DECREF(key); @@ -658,7 +677,7 @@ zipimporter_get_source(PyObject *obj, PyObject *args) { ZipImporter *self = (ZipImporter *)obj; - PyObject *toc_entry; + PyObject *toc_entry, *files; PyObject *fullname, *subname, *path, *fullpath; enum zi_module_info mi; FILE *fp; @@ -697,7 +716,14 @@ return NULL; } - toc_entry = PyDict_GetItem(self->files, fullpath); + files = PyDict_GetItem(zip_directory_cache, self->archive); + if (files == NULL) { + /* This should never happen as safely_reopen_archive() should + * have repopulated zip_directory_cache if needed. */ + Py_DECREF(fullpath); + return NULL; + } + toc_entry = PyDict_GetItem(files, fullpath); Py_DECREF(fullpath); if (toc_entry != NULL) { PyObject *res, *bytes; @@ -796,7 +822,6 @@ static PyMemberDef zipimporter_members[] = { {"archive", T_OBJECT, offsetof(ZipImporter, archive), READONLY}, {"prefix", T_OBJECT, offsetof(ZipImporter, prefix), READONLY}, - {"_files", T_OBJECT, offsetof(ZipImporter, files), READONLY}, {NULL} }; @@ -836,10 +861,9 @@ PyObject_GenericGetAttr, /* tp_getattro */ 0, /* tp_setattro */ 0, /* tp_as_buffer */ - Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE | - Py_TPFLAGS_HAVE_GC, /* tp_flags */ + Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE, /* tp_flags */ zipimporter_doc, /* tp_doc */ - zipimporter_traverse, /* tp_traverse */ + 0, /* tp_traverse */ 0, /* tp_clear */ 0, /* tp_richcompare */ 0, /* tp_weaklistoffset */ @@ -856,7 +880,7 @@ (initproc)zipimporter_init, /* tp_init */ PyType_GenericAlloc, /* tp_alloc */ PyType_GenericNew, /* tp_new */ - PyObject_GC_Del, /* tp_free */ + 0, /* tp_free */ }; @@ -956,11 +980,13 @@ fclose(fp); return NULL; } - Py_XDECREF(self->files); /* free the old value. */ - self->files = files; } Py_DECREF(stat_now); - } /* stat succeeded */ + } else { + if (Py_VerboseFlag) + PySys_FormatStderr("# zipimport: os.fstat failed on the " + "open %U file.\n", self->archive); + } return fp; } @@ -1445,7 +1471,7 @@ static time_t get_mtime_of_source(ZipImporter *self, PyObject *path) { - PyObject *toc_entry, *stripped; + PyObject *toc_entry, *stripped, *files; time_t mtime; /* strip 'c' or 'o' from *.py[co] */ @@ -1457,7 +1483,13 @@ if (stripped == NULL) return (time_t)-1; - toc_entry = PyDict_GetItem(self->files, stripped); + files = PyDict_GetItem(zip_directory_cache, self->archive); + if (files == NULL) { + /* This should never happen as safely_reopen_archive() from + * our only caller repopulated zip_directory_cache if needed. */ + return 0; + } + toc_entry = PyDict_GetItem(files, stripped); Py_DECREF(stripped); if (toc_entry != NULL && PyTuple_Check(toc_entry) && PyTuple_Size(toc_entry) == 8) { @@ -1520,6 +1552,7 @@ } for (zso = zip_searchorder; *zso->suffix; zso++) { + PyObject *files; code = NULL; fullpath = PyUnicode_FromFormat("%U%s", path, zso->suffix); @@ -1530,7 +1563,14 @@ PySys_FormatStderr("# trying %U%c%U\n", self->archive, (int)SEP, fullpath); - toc_entry = PyDict_GetItem(self->files, fullpath); + files = PyDict_GetItem(zip_directory_cache, self->archive); + if (files == NULL) { + /* This should never happen as safely_reopen_archive() should + * have repopulated zip_directory_cache if needed; and the GIL + * is being held. */ + goto exit; + } + toc_entry = PyDict_GetItem(files, fullpath); if (toc_entry != NULL) { time_t mtime = 0; int ispackage = zso->type & IS_PACKAGE;