Skip to content

Commit c2b0b12

Browse files
Marcel Plchncoghlan
Marcel Plch
authored andcommitted
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.
1 parent d6e1404 commit c2b0b12

File tree

5 files changed

+96
-7
lines changed

5 files changed

+96
-7
lines changed

Doc/c-api/module.rst

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -196,17 +196,23 @@ or request "multi-phase initialization" by returning the definition struct itsel
196196
.. c:member:: traverseproc m_traverse
197197
198198
A traversal function to call during GC traversal of the module object, or
199-
*NULL* if not needed.
199+
*NULL* if not needed. This function may be called before module state
200+
is allocated (:c:func:`PyModule_GetState()` may return `NULL`),
201+
and before the :c:member:`Py_mod_exec` function is executed.
200202
201203
.. c:member:: inquiry m_clear
202204
203205
A clear function to call during GC clearing of the module object, or
204-
*NULL* if not needed.
206+
*NULL* if not needed. This function may be called before module state
207+
is allocated (:c:func:`PyModule_GetState()` may return `NULL`),
208+
and before the :c:member:`Py_mod_exec` function is executed.
205209
206210
.. c:member:: freefunc m_free
207211
208212
A function to call during deallocation of the module object, or *NULL* if
209-
not needed.
213+
not needed. This function may be called before module state
214+
is allocated (:c:func:`PyModule_GetState()` may return `NULL`),
215+
and before the :c:member:`Py_mod_exec` function is executed.
210216
211217
Single-phase initialization
212218
...........................

Lib/test/test_importlib/extension/test_loader.py

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
import unittest
1010
import importlib.util
1111
import importlib
12-
12+
from test.support.script_helper import assert_python_failure
1313

1414
class LoaderTests(abc.LoaderTests):
1515

@@ -267,6 +267,20 @@ def test_nonascii(self):
267267
self.assertEqual(module.__name__, name)
268268
self.assertEqual(module.__doc__, "Module named in %s" % lang)
269269

270+
@unittest.skipIf(not hasattr(sys, 'gettotalrefcount'),
271+
'--with-pydebug has to be enabled for this test')
272+
def test_bad_traverse(self):
273+
''' Issue #32374: Test that traverse fails when accessing per-module
274+
state before Py_mod_exec was executed.
275+
(Multiphase initialization modules only)
276+
'''
277+
script = """if True:
278+
import importlib.util as util
279+
spec = util.find_spec('_testmultiphase')
280+
spec.name = '_testmultiphase_with_bad_traverse'
281+
m = spec.loader.create_module(spec)"""
282+
assert_python_failure("-c", script)
283+
270284

271285
(Frozen_MultiPhaseExtensionModuleTests,
272286
Source_MultiPhaseExtensionModuleTests
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Document that m_traverse for multi-phase initialized modules can be called
2+
with m_state=NULL, and add a sanity check

Modules/_testmultiphase.c

Lines changed: 49 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,10 @@ typedef struct {
1010
PyObject *x_attr; /* Attributes dictionary */
1111
} ExampleObject;
1212

13+
typedef struct {
14+
PyObject *integer;
15+
} testmultiphase_state;
16+
1317
/* Example methods */
1418

1519
static int
@@ -218,18 +222,21 @@ static int execfunc(PyObject *m)
218222
}
219223

220224
/* Helper for module definitions; there'll be a lot of them */
221-
#define TEST_MODULE_DEF(name, slots, methods) { \
225+
226+
#define TEST_MODULE_DEF_EX(name, slots, methods, statesize, traversefunc) { \
222227
PyModuleDef_HEAD_INIT, /* m_base */ \
223228
name, /* m_name */ \
224229
PyDoc_STR("Test module " name), /* m_doc */ \
225-
0, /* m_size */ \
230+
statesize, /* m_size */ \
226231
methods, /* m_methods */ \
227232
slots, /* m_slots */ \
228-
NULL, /* m_traverse */ \
233+
traversefunc, /* m_traverse */ \
229234
NULL, /* m_clear */ \
230235
NULL, /* m_free */ \
231236
}
232237

238+
#define TEST_MODULE_DEF(name, slots, methods) TEST_MODULE_DEF_EX(name, slots, methods, 0, NULL)
239+
233240
PyModuleDef_Slot main_slots[] = {
234241
{Py_mod_exec, execfunc},
235242
{0, NULL},
@@ -613,6 +620,44 @@ PyInit__testmultiphase_exec_unreported_exception(PyObject *spec)
613620
return PyModuleDef_Init(&def_exec_unreported_exception);
614621
}
615622

623+
static int
624+
bad_traverse(PyObject *self, visitproc visit, void *arg) {
625+
testmultiphase_state *m_state;
626+
627+
m_state = PyModule_GetState(self);
628+
Py_VISIT(m_state->integer);
629+
return 0;
630+
}
631+
632+
static int
633+
execfunc_with_bad_traverse(PyObject *mod) {
634+
testmultiphase_state *m_state;
635+
636+
m_state = PyModule_GetState(mod);
637+
if (m_state == NULL) {
638+
return -1;
639+
}
640+
641+
m_state->integer = PyLong_FromLong(0x7fffffff);
642+
Py_INCREF(m_state->integer);
643+
644+
return 0;
645+
}
646+
647+
static PyModuleDef_Slot slots_with_bad_traverse[] = {
648+
{Py_mod_exec, execfunc_with_bad_traverse},
649+
{0, NULL}
650+
};
651+
652+
static PyModuleDef def_with_bad_traverse = TEST_MODULE_DEF_EX(
653+
"_testmultiphase_with_bad_traverse", slots_with_bad_traverse, NULL,
654+
sizeof(testmultiphase_state), bad_traverse);
655+
656+
PyMODINIT_FUNC
657+
PyInit__testmultiphase_with_bad_traverse(PyObject *spec) {
658+
return PyModuleDef_Init(&def_with_bad_traverse);
659+
}
660+
616661
/*** Helper for imp test ***/
617662

618663
static PyModuleDef imp_dummy_def = TEST_MODULE_DEF("imp_dummy", main_slots, testexport_methods);
@@ -622,3 +667,4 @@ PyInit_imp_dummy(PyObject *spec)
622667
{
623668
return PyModuleDef_Init(&imp_dummy_def);
624669
}
670+

Objects/moduleobject.c

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,17 @@ static PyMemberDef module_members[] = {
2121
{0}
2222
};
2323

24+
25+
/* Helper for sanity check for traverse not handling m_state == NULL
26+
* Issue #32374 */
27+
#ifdef Py_DEBUG
28+
static int
29+
bad_traverse_test(PyObject *self, void *arg) {
30+
assert(self != NULL);
31+
return 0;
32+
}
33+
#endif
34+
2435
PyTypeObject PyModuleDef_Type = {
2536
PyVarObject_HEAD_INIT(&PyType_Type, 0)
2637
"moduledef", /* tp_name */
@@ -345,6 +356,16 @@ PyModule_FromDefAndSpec2(struct PyModuleDef* def, PyObject *spec, int module_api
345356
}
346357
}
347358

359+
/* Sanity check for traverse not handling m_state == NULL
360+
* This doesn't catch all possible cases, but in many cases it should
361+
* make many cases of invalid code crash or raise Valgrind issues
362+
* sooner than they would otherwise.
363+
* Issue #32374 */
364+
#ifdef Py_DEBUG
365+
if (def->m_traverse != NULL) {
366+
def->m_traverse(m, bad_traverse_test, NULL);
367+
}
368+
#endif
348369
Py_DECREF(nameobj);
349370
return m;
350371

0 commit comments

Comments
 (0)