From 8bbf7c593ddbf6f7740c43df389681f3d69dfa3c Mon Sep 17 00:00:00 2001 From: Marcel Plch Date: Fri, 22 Dec 2017 22:38:39 +0100 Subject: [PATCH 1/2] Documentation and testing --- Doc/c-api/module.rst | 6 +-- .../test_importlib/extension/test_loader.py | 12 ++++- Modules/_testmultiphase.c | 52 +++++++++++++++++-- Objects/moduleobject.c | 13 +++++ 4 files changed, 76 insertions(+), 7 deletions(-) diff --git a/Doc/c-api/module.rst b/Doc/c-api/module.rst index 7efab28af724aa..d665bd686cb71a 100644 --- a/Doc/c-api/module.rst +++ b/Doc/c-api/module.rst @@ -196,17 +196,17 @@ 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. If m_state is processed within this function, NULL check is needed. .. 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. If m_state is processed within this function, NULL check is needed. .. c:member:: freefunc m_free A function to call during deallocation of the module object, or *NULL* if - not needed. + not needed. If m_state is processed within this function, NULL check is needed. Single-phase initialization ........................... diff --git a/Lib/test/test_importlib/extension/test_loader.py b/Lib/test/test_importlib/extension/test_loader.py index 8d20040aab8d9a..8668dfa8f68fee 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,16 @@ 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): + 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/Modules/_testmultiphase.c b/Modules/_testmultiphase.c index 9b04ebf15586c1..8090a485f4a9f1 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 d6cde4024336c5..2a2784a1c0b490 100644 --- a/Objects/moduleobject.c +++ b/Objects/moduleobject.c @@ -21,6 +21,14 @@ static PyMemberDef module_members[] = { {0} }; +#ifdef Py_DEBUG +static int +PyTestVisit(PyObject *self, void *arg) { + assert(self != NULL); + return 0; +} +#endif + PyTypeObject PyModuleDef_Type = { PyVarObject_HEAD_INIT(&PyType_Type, 0) "moduledef", /* tp_name */ @@ -345,6 +353,11 @@ PyModule_FromDefAndSpec2(struct PyModuleDef* def, PyObject *spec, int module_api } } +#ifdef Py_DEBUG + if (def->m_traverse != NULL) { + def->m_traverse(m, bad_traverse_test, NULL); + } +#endif Py_DECREF(nameobj); return m; From 033521427bee78339680221e34604974719ebc33 Mon Sep 17 00:00:00 2001 From: Marcel Plch Date: Mon, 8 Jan 2018 17:08:50 +0100 Subject: [PATCH 2/2] fixes --- Doc/c-api/module.rst | 12 +++++++++--- Lib/test/test_importlib/extension/test_loader.py | 4 ++++ .../C API/2018-01-09-17-03-54.bpo-32374.SwwLoz.rst | 2 ++ Objects/moduleobject.c | 10 +++++++++- 4 files changed, 24 insertions(+), 4 deletions(-) create mode 100644 Misc/NEWS.d/next/C API/2018-01-09-17-03-54.bpo-32374.SwwLoz.rst diff --git a/Doc/c-api/module.rst b/Doc/c-api/module.rst index d665bd686cb71a..017b656854a8cd 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. If m_state is processed within this function, NULL check is 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. If m_state is processed within this function, NULL check is 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. If m_state is processed within this function, NULL check is 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 8668dfa8f68fee..53ac3c71d4b500 100644 --- a/Lib/test/test_importlib/extension/test_loader.py +++ b/Lib/test/test_importlib/extension/test_loader.py @@ -270,6 +270,10 @@ def test_nonascii(self): @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') 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 00000000000000..f9cf6d6b99ce9d --- /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/Objects/moduleobject.c b/Objects/moduleobject.c index 2a2784a1c0b490..8fb368e41474a8 100644 --- a/Objects/moduleobject.c +++ b/Objects/moduleobject.c @@ -21,9 +21,12 @@ static PyMemberDef module_members[] = { {0} }; + +/* Helper for sanity check for traverse not handling m_state == NULL + * Issue #32374 */ #ifdef Py_DEBUG static int -PyTestVisit(PyObject *self, void *arg) { +bad_traverse_test(PyObject *self, void *arg) { assert(self != NULL); return 0; } @@ -353,6 +356,11 @@ 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);