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.
(cherry picked from commit c2b0b12)

Co-authored-by: Marcel Plch <[email protected]>

Author: miss-islington

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
 ...........................

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

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

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
@@ -219,18 +223,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},
@@ -614,6 +621,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);
@@ -623,3 +668,4 @@ PyInit_imp_dummy(PyObject *spec)
 {
     return PyModuleDef_Init(&imp_dummy_def);
 }
+

Objects/moduleobject.c

@@ -20,6 +20,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 */
@@ -338,6 +349,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;