[Python-checkins] bpo-32374: m_traverse may be called with m_state=NULL (GH-5140)

Sat Mar 17 01:41:30 EDT 2018

https://github.com/python/cpython/commit/c2b0b12d1a137ada1023ab7c10b8d9a0249d95f9
commit: c2b0b12d1a137ada1023ab7c10b8d9a0249d95f9
branch: master
author: Marcel Plch <gmarcel.plch at gmail.com>
committer: Nick Coghlan <ncoghlan at gmail.com>
date: 2018年03月17日T15:41:20+10:00
summary:
bpo-32374: m_traverse may be called with m_state=NULL (GH-5140)
Multi-phase initialized modules allow m_traverse to be called while the
module is still being initialized, so module authors may need to account
for that.
files:
A Misc/NEWS.d/next/C API/2018-01-09-17-03-54.bpo-32374.SwwLoz.rst
M Doc/c-api/module.rst
M Lib/test/test_importlib/extension/test_loader.py
M Modules/_testmultiphase.c
M Objects/moduleobject.c

diff --git a/Doc/c-api/module.rst b/Doc/c-api/module.rst
index 7efab28af724..017b656854a8 100644
--- a/Doc/c-api/module.rst
+++ b/Doc/c-api/module.rst
@@ -196,17 +196,23 @@ or request "multi-phase initialization" by returning the definition struct itsel
 .. c:member:: traverseproc m_traverse
 
 A traversal function to call during GC traversal of the module object, or
- *NULL* if not needed.
+ *NULL* if not needed. This function may be called before module state
+ is allocated (:c:func:`PyModule_GetState()` may return `NULL`),
+ and before the :c:member:`Py_mod_exec` function is executed.
 
 .. c:member:: inquiry m_clear
 
 A clear function to call during GC clearing of the module object, or
- *NULL* if not needed.
+ *NULL* if not needed. This function may be called before module state
+ is allocated (:c:func:`PyModule_GetState()` may return `NULL`),
+ and before the :c:member:`Py_mod_exec` function is executed.
 
 .. c:member:: freefunc m_free
 
 A function to call during deallocation of the module object, or *NULL* if
- not needed.
+ not needed. This function may be called before module state
+ is allocated (:c:func:`PyModule_GetState()` may return `NULL`),
+ and before the :c:member:`Py_mod_exec` function is executed.
 
 Single-phase initialization
 ...........................
diff --git a/Lib/test/test_importlib/extension/test_loader.py b/Lib/test/test_importlib/extension/test_loader.py
index 8d20040aab8d..53ac3c71d4b5 100644
--- a/Lib/test/test_importlib/extension/test_loader.py
+++ b/Lib/test/test_importlib/extension/test_loader.py
@@ -9,7 +9,7 @@
 import unittest
 import importlib.util
 import importlib
-
+from test.support.script_helper import assert_python_failure
 
 class LoaderTests(abc.LoaderTests):
 
@@ -267,6 +267,20 @@ def test_nonascii(self):
 self.assertEqual(module.__name__, name)
 self.assertEqual(module.__doc__, "Module named in %s" % lang)
 
+ @unittest.skipIf(not hasattr(sys, 'gettotalrefcount'),
+ '--with-pydebug has to be enabled for this test')
+ def test_bad_traverse(self):
+ ''' Issue #32374: Test that traverse fails when accessing per-module
+ state before Py_mod_exec was executed.
+ (Multiphase initialization modules only)
+ '''
+ script = """if True:
+ import importlib.util as util
+ spec = util.find_spec('_testmultiphase')
+ spec.name = '_testmultiphase_with_bad_traverse'
+ m = spec.loader.create_module(spec)"""
+ assert_python_failure("-c", script)
+
 
 (Frozen_MultiPhaseExtensionModuleTests,
 Source_MultiPhaseExtensionModuleTests
diff --git a/Misc/NEWS.d/next/C API/2018-01-09-17-03-54.bpo-32374.SwwLoz.rst b/Misc/NEWS.d/next/C API/2018-01-09-17-03-54.bpo-32374.SwwLoz.rst
new file mode 100644
index 000000000000..f9cf6d6b99ce
--- /dev/null
+++ b/Misc/NEWS.d/next/C API/2018-01-09-17-03-54.bpo-32374.SwwLoz.rst	
@@ -0,0 +1,2 @@
+Document that m_traverse for multi-phase initialized modules can be called
+with m_state=NULL, and add a sanity check
diff --git a/Modules/_testmultiphase.c b/Modules/_testmultiphase.c
index 9b04ebf15586..8090a485f4a9 100644
--- a/Modules/_testmultiphase.c
+++ b/Modules/_testmultiphase.c
@@ -10,6 +10,10 @@ typedef struct {
 PyObject *x_attr; /* Attributes dictionary */
 } ExampleObject;
 
+typedef struct {
+ PyObject *integer;
+} testmultiphase_state;
+
 /* Example methods */
 
 static int
@@ -218,18 +222,21 @@ static int execfunc(PyObject *m)
 }
 
 /* Helper for module definitions; there'll be a lot of them */
-#define TEST_MODULE_DEF(name, slots, methods) { \
+
+#define TEST_MODULE_DEF_EX(name, slots, methods, statesize, traversefunc) { \
 PyModuleDef_HEAD_INIT, /* m_base */ \
 name, /* m_name */ \
 PyDoc_STR("Test module " name), /* m_doc */ \
- 0, /* m_size */ \
+ statesize, /* m_size */ \
 methods, /* m_methods */ \
 slots, /* m_slots */ \
- NULL, /* m_traverse */ \
+ traversefunc, /* m_traverse */ \
 NULL, /* m_clear */ \
 NULL, /* m_free */ \
 }
 
+#define TEST_MODULE_DEF(name, slots, methods) TEST_MODULE_DEF_EX(name, slots, methods, 0, NULL)
+
 PyModuleDef_Slot main_slots[] = {
 {Py_mod_exec, execfunc},
 {0, NULL},
@@ -613,6 +620,44 @@ PyInit__testmultiphase_exec_unreported_exception(PyObject *spec)
 return PyModuleDef_Init(&def_exec_unreported_exception);
 }
 
+static int
+bad_traverse(PyObject *self, visitproc visit, void *arg) {
+ testmultiphase_state *m_state;
+
+ m_state = PyModule_GetState(self);
+ Py_VISIT(m_state->integer);
+ return 0;
+}
+
+static int
+execfunc_with_bad_traverse(PyObject *mod) {
+ testmultiphase_state *m_state;
+
+ m_state = PyModule_GetState(mod);
+ if (m_state == NULL) {
+ return -1;
+ }
+
+ m_state->integer = PyLong_FromLong(0x7fffffff);
+ Py_INCREF(m_state->integer);
+
+ return 0;
+}
+
+static PyModuleDef_Slot slots_with_bad_traverse[] = {
+ {Py_mod_exec, execfunc_with_bad_traverse},
+ {0, NULL}
+};
+
+static PyModuleDef def_with_bad_traverse = TEST_MODULE_DEF_EX(
+ "_testmultiphase_with_bad_traverse", slots_with_bad_traverse, NULL,
+ sizeof(testmultiphase_state), bad_traverse);
+
+PyMODINIT_FUNC
+PyInit__testmultiphase_with_bad_traverse(PyObject *spec) {
+ return PyModuleDef_Init(&def_with_bad_traverse);
+}
+
 /*** Helper for imp test ***/
 
 static PyModuleDef imp_dummy_def = TEST_MODULE_DEF("imp_dummy", main_slots, testexport_methods);
@@ -622,3 +667,4 @@ PyInit_imp_dummy(PyObject *spec)
 {
 return PyModuleDef_Init(&imp_dummy_def);
 }
+
diff --git a/Objects/moduleobject.c b/Objects/moduleobject.c
index d6cde4024336..8fb368e41474 100644
--- a/Objects/moduleobject.c
+++ b/Objects/moduleobject.c
@@ -21,6 +21,17 @@ static PyMemberDef module_members[] = {
 {0}
 };
 
+
+/* Helper for sanity check for traverse not handling m_state == NULL
+ * Issue #32374 */
+#ifdef Py_DEBUG
+static int
+bad_traverse_test(PyObject *self, void *arg) {
+ assert(self != NULL);
+ return 0;
+}
+#endif
+
 PyTypeObject PyModuleDef_Type = {
 PyVarObject_HEAD_INIT(&PyType_Type, 0)
 "moduledef", /* tp_name */
@@ -345,6 +356,16 @@ PyModule_FromDefAndSpec2(struct PyModuleDef* def, PyObject *spec, int module_api
 }
 }
 
+ /* Sanity check for traverse not handling m_state == NULL
+ * This doesn't catch all possible cases, but in many cases it should
+ * make many cases of invalid code crash or raise Valgrind issues
+ * sooner than they would otherwise.
+ * Issue #32374 */
+#ifdef Py_DEBUG
+ if (def->m_traverse != NULL) {
+ def->m_traverse(m, bad_traverse_test, NULL);
+ }
+#endif
 Py_DECREF(nameobj);
 return m;
 
