[Python-checkins] cpython (2.7): backout #19081 to fix #20621

Sun Feb 16 20:21:50 CET 2014

http://hg.python.org/cpython/rev/3dd8b0d31543
changeset: 89223:3dd8b0d31543
branch: 2.7
parent: 89202:a87f284e14ea
user: Benjamin Peterson <benjamin at python.org>
date: Sun Feb 16 14:20:14 2014 -0500
summary:
 backout #19081 to fix #20621
files:
 Lib/test/test_zipimport.py | 132 +++----------------
 Misc/NEWS | 5 -
 Modules/zipimport.c | 168 +++++++++---------------
 3 files changed, 88 insertions(+), 217 deletions(-)

diff --git a/Lib/test/test_zipimport.py b/Lib/test/test_zipimport.py
--- a/Lib/test/test_zipimport.py
+++ b/Lib/test/test_zipimport.py
@@ -395,145 +395,57 @@
 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 setUpZipFileModuleAndTestImports(self):
- # Create a .zip file to test with
- self.zipfile_path = TEMP_ZIP
+ def testZipFileChangesAfterFirstImport(self):
+ """Alter the zip file after caching its index and try an import."""
 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")}
- _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()
+ zipfile_path = TEMP_ZIP
+ _write_zip_package(zipfile_path, files)
+ self.assertTrue(os.path.exists(zipfile_path))
+ sys.path.insert(0, zipfile_path)
 
 # Import something out of the zipfile and confirm it is correct.
- testmod = __import__(self.testpack_testmod,
+ testmod = __import__(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"]
 
- def truncateAndFillZipWithNonZipGarbage(self):
- with io.open(self.zipfile_path, "wb") as byebye_valid_zip_file:
+ # Truncate and fill the zip file with non-zip garbage.
+ with io.open(zipfile_path, "rb") as orig_zip_file:
+ orig_zip_file_contents = orig_zip_file.read()
+ with io.open(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", {}, {}, ["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), {})
+ ziptest_a = __import__("ziptest_a", globals(), locals(),
+ ["test_value"])
 
- # Alters all of the offsets within the file
- self.restoreZipFileWithDifferentHeaderOffsets()
+ # Now lets make it a valid zipfile that has some garbage at the start.
+ # This alters all of the offsets within the file
+ with io.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)
 
 # Now that the zip file has been "restored" to a valid but different
- # 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_a = __import__("ziptest_a", {}, {}, ["test_value"])
+ # 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_a = __import__("ziptest_a", globals(), locals(), ["test_value"])
 self.assertEqual(ziptest_a.test_value, 23)
- 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
+ ziptest_c = __import__("ziptest_c", globals(), locals(), ["test_value"])
 self.assertEqual(ziptest_c.test_value, 1337)
 
 
diff --git a/Misc/NEWS b/Misc/NEWS
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -18,11 +18,6 @@
 - Issue #17825: Cursor "^" is correctly positioned for SyntaxError and
 IndentationError.
 
-- Issue #19081: When a zipimport .zip file in sys.path being imported from
- is modified during the lifetime of the Python process after zipimport has
- already cached the zip's table of contents we detect this and recover
- rather than read bad data from the .zip (causing odd import errors).
-
 - Raise a better error when non-unicode codecs are used for a file's coding
 cookie.
 
diff --git a/Modules/zipimport.c b/Modules/zipimport.c
--- a/Modules/zipimport.c
+++ b/Modules/zipimport.c
@@ -66,34 +66,30 @@
 static int
 zipimporter_init(ZipImporter *self, PyObject *args, PyObject *kwds)
 {
- char *path_arg, *path, *p, *prefix, *path_buf;
+ char *path, *p, *prefix, buf[MAXPATHLEN+2];
 size_t len;
 
 if (!_PyArg_NoKeywords("zipimporter()", kwds))
 return -1;
 
- if (!PyArg_ParseTuple(args, "s:zipimporter", &path_arg))
+ if (!PyArg_ParseTuple(args, "s:zipimporter",
+ &path))
 return -1;
 
- len = strlen(path_arg);
+ len = strlen(path);
 if (len == 0) {
 PyErr_SetString(ZipImportError, "archive path is empty");
 return -1;
 }
 if (len >= MAXPATHLEN) {
- PyErr_SetString(ZipImportError, "archive path too long");
+ PyErr_SetString(ZipImportError,
+ "archive path too long");
 return -1;
 }
- /* Room for the trailing 0円 and room for an extra SEP if needed. */
- path_buf = (char *)PyMem_Malloc(len + 2);
- if (path_buf == NULL) {
- PyErr_SetString(PyExc_MemoryError, "unable to malloc path buffer");
- return -1;
- }
- strcpy(path_buf, path_arg);
+ strcpy(buf, path);
 
 #ifdef ALTSEP
- for (p = path_buf; *p; p++) {
+ for (p = buf; *p; p++) {
 if (*p == ALTSEP)
 *p = SEP;
 }
@@ -106,25 +102,25 @@
 struct stat statbuf;
 int rv;
 
- rv = stat(path_buf, &statbuf);
+ rv = stat(buf, &statbuf);
 if (rv == 0) {
 /* it exists */
 if (S_ISREG(statbuf.st_mode))
 /* it's a file */
- path = path_buf;
+ path = buf;
 break;
 }
 #else
- if (object_exists(path_buf)) {
+ if (object_exists(buf)) {
 /* it exists */
- if (isfile(path_buf))
+ if (isfile(buf))
 /* it's a file */
- path = path_buf;
+ path = buf;
 break;
 }
 #endif
 /* back up one path element */
- p = strrchr(path_buf, SEP);
+ p = strrchr(buf, SEP);
 if (prefix != NULL)
 *prefix = SEP;
 if (p == NULL)
@@ -137,42 +133,45 @@
 files = PyDict_GetItemString(zip_directory_cache, path);
 if (files == NULL) {
 PyObject *zip_stat = NULL;
- FILE *fp = fopen_rb_and_stat(path, &zip_stat);
+ FILE *fp = fopen_rb_and_stat(buf, &zip_stat);
 if (fp == NULL) {
 PyErr_Format(ZipImportError, "can't open Zip file: "
- "'%.200s'", path);
+ "'%.200s'", buf);
 Py_XDECREF(zip_stat);
- goto error;
+ return -1;
 }
 
 if (Py_VerboseFlag)
 PySys_WriteStderr("# zipimport: %s not cached, "
 "reading TOC.\n", path);
 
- files = read_directory(fp, path);
+ files = read_directory(fp, buf);
 fclose(fp);
 if (files == NULL) {
 Py_XDECREF(zip_stat);
- goto error;
+ return -1;
 }
 if (PyDict_SetItemString(zip_directory_cache, path,
 files) != 0) {
 Py_DECREF(files);
 Py_XDECREF(zip_stat);
- goto error;
+ return -1;
 }
 if (zip_stat && PyDict_SetItemString(zip_stat_cache, path,
 zip_stat) != 0) {
 Py_DECREF(files);
 Py_DECREF(zip_stat);
- goto error;
+ return -1;
 }
 Py_XDECREF(zip_stat);
 }
+ else
+ Py_INCREF(files);
+ self->files = files;
 }
 else {
 PyErr_SetString(ZipImportError, "not a Zip file");
- goto error;
+ return -1;
 }
 
 if (prefix == NULL)
@@ -187,26 +186,33 @@
 }
 }
 
- self->archive = PyString_FromString(path);
+ self->archive = PyString_FromString(buf);
 if (self->archive == NULL)
- goto error;
+ return -1;
 
 self->prefix = PyString_FromString(prefix);
 if (self->prefix == NULL)
- goto error;
+ return -1;
 
- PyMem_Free(path_buf);
 return 0;
-error:
- PyMem_Free(path_buf);
- 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);
 }
 
@@ -286,7 +292,6 @@
 char *subname, path[MAXPATHLEN + 1];
 int len;
 struct st_zip_searchorder *zso;
- PyObject *files;
 
 subname = get_subname(fullname);
 
@@ -294,23 +299,9 @@
 if (len < 0)
 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. */
- char *unused_archive_name;
- FILE *fp = safely_reopen_archive(self, &unused_archive_name);
- 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++) {
 strcpy(path + len, zso->suffix);
- if (PyDict_GetItemString(files, path) != NULL) {
+ if (PyDict_GetItemString(self->files, path) != NULL) {
 if (zso->type & IS_PACKAGE)
 return MI_PACKAGE;
 else
@@ -465,7 +456,7 @@
 #ifdef ALTSEP
 char *p, buf[MAXPATHLEN + 1];
 #endif
- PyObject *toc_entry, *data, *files;
+ PyObject *toc_entry, *data;
 Py_ssize_t len;
 
 if (!PyArg_ParseTuple(args, "s:zipimporter.get_data", &path))
@@ -494,13 +485,7 @@
 if (fp == NULL)
 return NULL;
 
- 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. */
- return NULL;
- }
- toc_entry = PyDict_GetItemString(files, path);
+ toc_entry = PyDict_GetItemString(self->files, path);
 if (toc_entry == NULL) {
 PyErr_SetFromErrnoWithFilename(PyExc_IOError, path);
 fclose(fp);
@@ -527,7 +512,7 @@
 zipimporter_get_source(PyObject *obj, PyObject *args)
 {
 ZipImporter *self = (ZipImporter *)obj;
- PyObject *toc_entry, *files;
+ PyObject *toc_entry;
 FILE *fp;
 char *fullname, *subname, path[MAXPATHLEN+1], *archive;
 int len;
@@ -561,13 +546,7 @@
 if (fp == NULL)
 return NULL;
 
- 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. */
- return NULL;
- }
- toc_entry = PyDict_GetItemString(files, path);
+ toc_entry = PyDict_GetItemString(self->files, path);
 if (toc_entry != NULL) {
 PyObject *data = get_data(fp, archive, toc_entry);
 fclose(fp);
@@ -687,9 +666,10 @@
 PyObject_GenericGetAttr, /* tp_getattro */
 0, /* tp_setattro */
 0, /* tp_as_buffer */
- Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE, /* tp_flags */
+ Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE |
+ Py_TPFLAGS_HAVE_GC, /* tp_flags */
 zipimporter_doc, /* tp_doc */
- 0, /* tp_traverse */
+ zipimporter_traverse, /* tp_traverse */
 0, /* tp_clear */
 0, /* tp_richcompare */
 0, /* tp_weaklistoffset */
@@ -706,7 +686,7 @@
 (initproc)zipimporter_init, /* tp_init */
 PyType_GenericAlloc, /* tp_alloc */
 PyType_GenericNew, /* tp_new */
- 0, /* tp_free */
+ PyObject_GC_Del, /* tp_free */
 };
 
 
@@ -850,13 +830,13 @@
 fclose(fp);
 return NULL;
 }
+ Py_XDECREF(self->files); /* free the old value. */
+ self->files = files;
+ } else {
+ /* No problem, discard the new stat data. */
+ Py_DECREF(stat_now);
 }
- Py_DECREF(stat_now);
- } else {
- if (Py_VerboseFlag)
- PySys_WriteStderr("# zipimport: os.fstat failed on the "
- "open %s file.\n", archive);
- }
+ } /* stat succeeded */
 
 return fp;
 }
@@ -1258,18 +1238,12 @@
 static time_t
 get_mtime_of_source(ZipImporter *self, char *path)
 {
- PyObject *toc_entry, *files;
+ PyObject *toc_entry;
 time_t mtime = 0;
 Py_ssize_t lastchar = strlen(path) - 1;
 char savechar = path[lastchar];
 path[lastchar] = '0円'; /* strip 'c' or 'o' from *.py[co] */
- 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_GetItemString(files, path);
+ toc_entry = PyDict_GetItemString(self->files, path);
 if (toc_entry != NULL && PyTuple_Check(toc_entry) &&
 PyTuple_Size(toc_entry) == 8) {
 /* fetch the time stamp of the .py file for comparison
@@ -1318,8 +1292,6 @@
 char *subname, path[MAXPATHLEN + 1];
 int len;
 struct st_zip_searchorder *zso;
- FILE *fp;
- char *archive;
 
 subname = get_subname(fullname);
 
@@ -1327,12 +1299,10 @@
 if (len < 0)
 return NULL;
 
- fp = safely_reopen_archive(self, &archive);
- if (fp == NULL)
- return NULL;
-
 for (zso = zip_searchorder; *zso->suffix; zso++) {
- PyObject *code = NULL, *files;
+ PyObject *code = NULL;
+ FILE *fp;
+ char *archive;
 
 strcpy(path + len, zso->suffix);
 if (Py_VerboseFlag > 1)
@@ -1340,14 +1310,11 @@
 PyString_AsString(self->archive),
 SEP, path);
 
- 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. */
+ fp = safely_reopen_archive(self, &archive);
+ if (fp == NULL)
 return NULL;
- }
- toc_entry = PyDict_GetItemString(files, path);
+
+ toc_entry = PyDict_GetItemString(self->files, path);
 if (toc_entry != NULL) {
 time_t mtime = 0;
 int ispackage = zso->type & IS_PACKAGE;
@@ -1360,6 +1327,7 @@
 code = get_code_from_data(archive, fp, ispackage,
 isbytecode, mtime,
 toc_entry);
+ fclose(fp);
 if (code == Py_None) {
 /* bad magic number or non-matching mtime
 in byte code, try next */
@@ -1369,12 +1337,11 @@
 if (code != NULL && p_modpath != NULL)
 *p_modpath = PyString_AsString(
 PyTuple_GetItem(toc_entry, 0));
- fclose(fp);
 return code;
 }
+ fclose(fp);
 }
 PyErr_Format(ZipImportError, "can't find module '%.200s'", fullname);
- fclose(fp);
 return NULL;
 }
 
@@ -1462,10 +1429,9 @@
 * live within a zipped up standard library. Import the posix or nt
 * builtin that provides the fstat() function we want instead. */
 PyObject *os_like_module;
- Py_CLEAR(fstat_function); /* Avoid embedded interpreter leaks. */
+ Py_XDECREF(fstat_function); /* Avoid embedded interpreter leaks. */
 os_like_module = PyImport_ImportModule("posix");
 if (os_like_module == NULL) {
- PyErr_Clear();
 os_like_module = PyImport_ImportModule("nt");
 }
 if (os_like_module != NULL) {
@@ -1474,8 +1440,6 @@
 }
 if (fstat_function == NULL) {
 PyErr_Clear(); /* non-fatal, we'll go on without it. */
- if (Py_VerboseFlag)
- PySys_WriteStderr("# zipimport unable to use os.fstat().\n");
 }
 }
 }
-- 
Repository URL: http://hg.python.org/cpython
