diff --git a/Doc/c-api/module.rst b/Doc/c-api/module.rst index 57902a9c7f8..8d1a0fbeb76 100644 --- a/Doc/c-api/module.rst +++ b/Doc/c-api/module.rst @@ -196,23 +196,47 @@ 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. 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. + ``NULL`` if not needed. + + This function is not called if the module state was requested but is not + allocated yet. This is the case immediately after the module is created + and before the module is executed (:c:data:`Py_mod_exec` function). More + precisely, this function is not called if :c:member:`m_size` is greater + than 0 and the module state (as returned by :c:func:`PyModule_GetState`) + is ``NULL``. + + .. versionchanged:: 3.9 + No longer called before the module state is allocated. .. c:member:: inquiry m_clear A clear function to call during GC clearing of the module object, or - ``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. + ``NULL`` if not needed. + + This function is not called if the module state was requested but is not + allocated yet. This is the case immediately after the module is created + and before the module is executed (:c:data:`Py_mod_exec` function). More + precisely, this function is not called if :c:member:`m_size` is greater + than 0 and the module state (as returned by :c:func:`PyModule_GetState`) + is ``NULL``. + + .. versionchanged:: 3.9 + No longer called before the module state is allocated. .. c:member:: freefunc m_free - A function to call during deallocation of the module object, or ``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. + A function to call during deallocation of the module object, or ``NULL`` + if not needed. + + This function is not called if the module state was requested but is not + allocated yet. This is the case immediately after the module is created + and before the module is executed (:c:data:`Py_mod_exec` function). More + precisely, this function is not called if :c:member:`m_size` is greater + than 0 and the module state (as returned by :c:func:`PyModule_GetState`) + is ``NULL``. + + .. versionchanged:: 3.9 + No longer called before the module state is allocated. Single-phase initialization ........................... diff --git a/Doc/whatsnew/3.9.rst b/Doc/whatsnew/3.9.rst index 8c087c622e4..fd3d333f42f 100644 --- a/Doc/whatsnew/3.9.rst +++ b/Doc/whatsnew/3.9.rst @@ -503,6 +503,17 @@ Build and C API Changes *tstate* parameter (``PyThreadState*``). (Contributed by Victor Stinner in :issue:`38500`.) +* Extension modules: :c:member:`~PyModuleDef.m_traverse`, + :c:member:`~PyModuleDef.m_clear` and :c:member:`~PyModuleDef.m_free` + functions of :c:type:`PyModuleDef` are no longer called if the module state + was requested but is not allocated yet. This is the case immediately after + the module is created and before the module is executed + (:c:data:`Py_mod_exec` function). More precisely, these functions are not called + if :c:member:`~PyModuleDef.m_size` is greater than 0 and the module state (as + returned by :c:func:`PyModule_GetState`) is ``NULL``. + + Extension modules without module state (``m_size <= 0``) are not affected. + Deprecated ========== diff --git a/Lib/test/test_importlib/extension/test_loader.py b/Lib/test/test_importlib/extension/test_loader.py index 9ad05fadef2..abd612fcd9b 100644 --- a/Lib/test/test_importlib/extension/test_loader.py +++ b/Lib/test/test_importlib/extension/test_loader.py @@ -267,29 +267,6 @@ 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: - try: - from test import support - import importlib.util as util - spec = util.find_spec('_testmultiphase') - spec.name = '_testmultiphase_with_bad_traverse' - - with support.SuppressCrashReport(): - m = spec.loader.create_module(spec) - except: - # Prevent Python-level exceptions from - # ending the process with non-zero status - # (We are testing for a crash in C-code) - pass""" - assert_python_failure("-c", script) - (Frozen_MultiPhaseExtensionModuleTests, Source_MultiPhaseExtensionModuleTests diff --git a/Misc/NEWS.d/next/C API/2020-03-02-11-29-45.bpo-39824.71_ZMn.rst b/Misc/NEWS.d/next/C API/2020-03-02-11-29-45.bpo-39824.71_ZMn.rst new file mode 100644 index 00000000000..ae0a872c93e --- /dev/null +++ b/Misc/NEWS.d/next/C API/2020-03-02-11-29-45.bpo-39824.71_ZMn.rst @@ -0,0 +1,10 @@ +Extension modules: :c:member:`~PyModuleDef.m_traverse`, +:c:member:`~PyModuleDef.m_clear` and :c:member:`~PyModuleDef.m_free` functions +of :c:type:`PyModuleDef` are no longer called if the module state was requested +but is not allocated yet. This is the case immediately after the module is +created and before the module is executed (:c:data:`Py_mod_exec` function). More +precisely, these functions are not called if :c:member:`~PyModuleDef.m_size` is +greater than 0 and the module state (as returned by +:c:func:`PyModule_GetState`) is ``NULL``. + +Extension modules without module state (``m_size <= 0``) are not affected. diff --git a/Modules/_localemodule.c b/Modules/_localemodule.c index f68debdb1da..d2202bcad9f 100644 --- a/Modules/_localemodule.c +++ b/Modules/_localemodule.c @@ -778,9 +778,7 @@ static int locale_traverse(PyObject *m, visitproc visit, void *arg) { _locale_state *state = (_locale_state*)PyModule_GetState(m); - if (state) { - Py_VISIT(state->Error); - } + Py_VISIT(state->Error); return 0; } @@ -788,9 +786,7 @@ static int locale_clear(PyObject *m) { _locale_state *state = (_locale_state*)PyModule_GetState(m); - if (state) { - Py_CLEAR(state->Error); - } + Py_CLEAR(state->Error); return 0; } diff --git a/Modules/_testmultiphase.c b/Modules/_testmultiphase.c index 4933abbabbe..eadc46fbf18 100644 --- a/Modules/_testmultiphase.c +++ b/Modules/_testmultiphase.c @@ -225,20 +225,18 @@ static int execfunc(PyObject *m) /* Helper for module definitions; there'll be a lot of them */ -#define TEST_MODULE_DEF_EX(name, slots, methods, statesize, traversefunc) { \ +#define TEST_MODULE_DEF(name, slots, methods) { \ PyModuleDef_HEAD_INIT, /* m_base */ \ name, /* m_name */ \ PyDoc_STR("Test module " name), /* m_doc */ \ - statesize, /* m_size */ \ + 0, /* m_size */ \ methods, /* m_methods */ \ slots, /* m_slots */ \ - traversefunc, /* m_traverse */ \ + NULL, /* m_traverse */ \ NULL, /* m_clear */ \ NULL, /* m_free */ \ } -#define TEST_MODULE_DEF(name, slots, methods) TEST_MODULE_DEF_EX(name, slots, methods, 0, NULL) - static PyModuleDef_Slot main_slots[] = { {Py_mod_exec, execfunc}, {0, NULL}, @@ -622,52 +620,6 @@ 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); - - /* The following assertion mimics any traversal function that doesn't correctly handle - * the case during module creation where the module state hasn't been created yet. - * - * The check that it is used to test only runs in debug mode, so it is OK that the - * assert() will get compiled out in fully optimised release builds. - */ - assert(m_state != NULL); - 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); diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c index c150e4429d5..8cef64ceb9b 100644 --- a/Modules/atexitmodule.c +++ b/Modules/atexitmodule.c @@ -229,15 +229,14 @@ atexit_m_traverse(PyObject *self, visitproc visit, void *arg) atexitmodule_state *modstate; modstate = (atexitmodule_state *)PyModule_GetState(self); - if (modstate != NULL) { - for (i = 0; i < modstate->ncallbacks; i++) { - atexit_callback *cb = modstate->atexit_callbacks[i]; - if (cb == NULL) - continue; - Py_VISIT(cb->func); - Py_VISIT(cb->args); - Py_VISIT(cb->kwargs); - } + + for (i = 0; i < modstate->ncallbacks; i++) { + atexit_callback *cb = modstate->atexit_callbacks[i]; + if (cb == NULL) + continue; + Py_VISIT(cb->func); + Py_VISIT(cb->args); + Py_VISIT(cb->kwargs); } return 0; } @@ -247,9 +246,7 @@ atexit_m_clear(PyObject *self) { atexitmodule_state *modstate; modstate = (atexitmodule_state *)PyModule_GetState(self); - if (modstate != NULL) { - atexit_cleanup(modstate); - } + atexit_cleanup(modstate); return 0; } @@ -258,10 +255,8 @@ atexit_free(PyObject *m) { atexitmodule_state *modstate; modstate = (atexitmodule_state *)PyModule_GetState(m); - if (modstate != NULL) { - atexit_cleanup(modstate); - PyMem_Free(modstate->atexit_callbacks); - } + atexit_cleanup(modstate); + PyMem_Free(modstate->atexit_callbacks); } PyDoc_STRVAR(atexit_unregister__doc__, diff --git a/Modules/audioop.c b/Modules/audioop.c index 467bd6362a6..64cf98137c8 100644 --- a/Modules/audioop.c +++ b/Modules/audioop.c @@ -1926,9 +1926,7 @@ static int audioop_traverse(PyObject *module, visitproc visit, void *arg) { audioop_state *state = (audioop_state *)PyModule_GetState(module); - if (state) { - Py_VISIT(state->AudioopError); - } + Py_VISIT(state->AudioopError); return 0; } @@ -1936,9 +1934,7 @@ static int audioop_clear(PyObject *module) { audioop_state *state = (audioop_state *)PyModule_GetState(module); - if (state) { - Py_CLEAR(state->AudioopError); - } + Py_CLEAR(state->AudioopError); return 0; } diff --git a/Modules/binascii.c b/Modules/binascii.c index c63f3baf96a..f59cb7dd660 100644 --- a/Modules/binascii.c +++ b/Modules/binascii.c @@ -1653,10 +1653,8 @@ static int binascii_traverse(PyObject *module, visitproc visit, void *arg) { binascii_state *state = get_binascii_state(module); - if (state) { - Py_VISIT(state->Error); - Py_VISIT(state->Incomplete); - } + Py_VISIT(state->Error); + Py_VISIT(state->Incomplete); return 0; } @@ -1664,10 +1662,8 @@ static int binascii_clear(PyObject *module) { binascii_state *state = get_binascii_state(module); - if (state) { - Py_CLEAR(state->Error); - Py_CLEAR(state->Incomplete); - } + Py_CLEAR(state->Error); + Py_CLEAR(state->Incomplete); return 0; } @@ -1686,7 +1682,7 @@ static struct PyModuleDef binasciimodule = { binascii_slots, binascii_traverse, binascii_clear, - binascii_free + binascii_free }; PyMODINIT_FUNC diff --git a/Objects/moduleobject.c b/Objects/moduleobject.c index c581951f7ae..f02ca75c9ee 100644 --- a/Objects/moduleobject.c +++ b/Objects/moduleobject.c @@ -26,16 +26,6 @@ static PyMemberDef module_members[] = { }; -/* 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 */ @@ -360,16 +350,6 @@ 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; @@ -687,8 +667,12 @@ module_dealloc(PyModuleObject *m) } if (m->md_weaklist != NULL) PyObject_ClearWeakRefs((PyObject *) m); - if (m->md_def && m->md_def->m_free) + /* bpo-39824: Don't call m_free() if m_size > 0 and md_state=NULL */ + if (m->md_def && m->md_def->m_free + && (m->md_def->m_size <= 0 || m->md_state != NULL)) + { m->md_def->m_free(m); + } Py_XDECREF(m->md_dict); Py_XDECREF(m->md_name); if (m->md_state != NULL) @@ -770,7 +754,10 @@ module_getattro(PyModuleObject *m, PyObject *name) static int module_traverse(PyModuleObject *m, visitproc visit, void *arg) { - if (m->md_def && m->md_def->m_traverse) { + /* bpo-39824: Don't call m_traverse() if m_size > 0 and md_state=NULL */ + if (m->md_def && m->md_def->m_traverse + && (m->md_def->m_size <= 0 || m->md_state != NULL)) + { int res = m->md_def->m_traverse((PyObject*)m, visit, arg); if (res) return res; @@ -782,7 +769,10 @@ module_traverse(PyModuleObject *m, visitproc visit, void *arg) static int module_clear(PyModuleObject *m) { - if (m->md_def && m->md_def->m_clear) { + /* bpo-39824: Don't call m_clear() if m_size > 0 and md_state=NULL */ + if (m->md_def && m->md_def->m_clear + && (m->md_def->m_size <= 0 || m->md_state != NULL)) + { int res = m->md_def->m_clear((PyObject*)m); if (PyErr_Occurred()) { PySys_FormatStderr("Exception ignored in m_clear of module%s%V\n",