From cc6784962b59f12e4d94d67fe6885d8e02776b20 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 7 Feb 2024 18:23:02 -0700 Subject: [PATCH 01/40] Factor out _PyImport_RunDynamicModule(). --- Include/internal/pycore_importdl.h | 25 +++- Python/importdl.c | 222 +++++++++++++++++++---------- 2 files changed, 171 insertions(+), 76 deletions(-) diff --git a/Include/internal/pycore_importdl.h b/Include/internal/pycore_importdl.h index c8583582b358ac..9683152736eb70 100644 --- a/Include/internal/pycore_importdl.h +++ b/Include/internal/pycore_importdl.h @@ -14,10 +14,31 @@ extern "C" { extern const char *_PyImport_DynLoadFiletab[]; -extern PyObject *_PyImport_LoadDynamicModuleWithSpec(PyObject *spec, FILE *); - typedef PyObject *(*PyModInitFunction)(void); +struct _Py_ext_module_loader_info { + PyObject *path; + PyObject *name; + PyObject *name_encoded; + const char *hook_prefix; +}; +extern void _Py_ext_module_loader_info_clear( + struct _Py_ext_module_loader_info *info); +extern int _Py_ext_module_loader_info_from_spec( + PyObject *spec, + struct _Py_ext_module_loader_info *info); + +struct _Py_ext_module_loader_result { + PyModuleDef *def; + PyObject *module; +}; +extern int _PyImport_RunDynamicModule( + struct _Py_ext_module_loader_info *info, + FILE *fp, + struct _Py_ext_module_loader_result *res); + +extern PyObject *_PyImport_LoadDynamicModuleWithSpec(PyObject *spec, FILE *); + /* Max length of module suffix searched for -- accommodates "module.slb" */ #define MAXSUFFIXSIZE 12 diff --git a/Python/importdl.c b/Python/importdl.c index 7dfd301d77efb4..bb9b068e1cec1a 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -93,58 +93,91 @@ get_encoded_name(PyObject *name, const char **hook_prefix) { return NULL; } -PyObject * -_PyImport_LoadDynamicModuleWithSpec(PyObject *spec, FILE *fp) +void +_Py_ext_module_loader_info_clear(struct _Py_ext_module_loader_info *info) { -#ifndef MS_WINDOWS - PyObject *pathbytes = NULL; -#endif - PyObject *name_unicode = NULL, *name = NULL, *path = NULL, *m = NULL; - const char *name_buf, *hook_prefix; - const char *oldcontext, *newcontext; - dl_funcptr exportfunc; - PyModuleDef *def; - PyModInitFunction p0; + Py_CLEAR(info->path); + Py_CLEAR(info->name); + Py_CLEAR(info->name_encoded); +} + +int +_Py_ext_module_loader_info_from_spec(PyObject *spec, + struct _Py_ext_module_loader_info *info) +{ + PyObject *name_unicode = NULL, *name = NULL, *path = NULL; + const char *hook_prefix; name_unicode = PyObject_GetAttrString(spec, "name"); if (name_unicode == NULL) { - return NULL; + return -1; } if (!PyUnicode_Check(name_unicode)) { PyErr_SetString(PyExc_TypeError, "spec.name must be a string"); - goto error; - } - newcontext = PyUnicode_AsUTF8(name_unicode); - if (newcontext == NULL) { - goto error; + Py_DECREF(name_unicode); + return -1; } name = get_encoded_name(name_unicode, &hook_prefix); if (name == NULL) { - goto error; + Py_DECREF(name_unicode); + return -1; } - name_buf = PyBytes_AS_STRING(name); path = PyObject_GetAttrString(spec, "origin"); - if (path == NULL) - goto error; + if (path == NULL) { + Py_DECREF(name_unicode); + Py_DECREF(name); + return -1; + } + + *info = (struct _Py_ext_module_loader_info){ + .path=path, + .name=name_unicode, + .name_encoded=name, + .hook_prefix=hook_prefix, + }; + return 0; +} + +int +_PyImport_RunDynamicModule(struct _Py_ext_module_loader_info *info, + FILE *fp, + struct _Py_ext_module_loader_result *res) +{ +#ifndef MS_WINDOWS + PyObject *pathbytes = NULL; + const char *path_buf; +#endif + PyObject *m = NULL; + const char *name_buf = PyBytes_AS_STRING(info->name_encoded); + const char *oldcontext, *newcontext; + dl_funcptr exportfunc; + PyModInitFunction p0; + PyModuleDef *def; + + newcontext = PyUnicode_AsUTF8(info->name); + if (newcontext == NULL) { + return -1; + } - if (PySys_Audit("import", "OOOOO", name_unicode, path, + if (PySys_Audit("import", "OOOOO", info->name, info->path, Py_None, Py_None, Py_None) < 0) { - goto error; + return -1; } #ifdef MS_WINDOWS - exportfunc = _PyImport_FindSharedFuncptrWindows(hook_prefix, name_buf, - path, fp); + exportfunc = _PyImport_FindSharedFuncptrWindows( + info->hook_prefix, name_buf, info->path, fp); #else - pathbytes = PyUnicode_EncodeFSDefault(path); - if (pathbytes == NULL) - goto error; - exportfunc = _PyImport_FindSharedFuncptr(hook_prefix, name_buf, - PyBytes_AS_STRING(pathbytes), - fp); + pathbytes = PyUnicode_EncodeFSDefault(info->path); + if (pathbytes == NULL) { + return -1; + } + path_buf = PyBytes_AS_STRING(pathbytes); + exportfunc = _PyImport_FindSharedFuncptr( + info->hook_prefix, name_buf, path_buf, fp); Py_DECREF(pathbytes); #endif @@ -154,13 +187,13 @@ _PyImport_LoadDynamicModuleWithSpec(PyObject *spec, FILE *fp) msg = PyUnicode_FromFormat( "dynamic module does not define " "module export function (%s_%s)", - hook_prefix, name_buf); - if (msg == NULL) - goto error; - PyErr_SetImportError(msg, name_unicode, path); - Py_DECREF(msg); + info->hook_prefix, name_buf); + if (msg != NULL) { + PyErr_SetImportError(msg, info->name, info->path); + Py_DECREF(msg); + } } - goto error; + return -1; } p0 = (PyModInitFunction)exportfunc; @@ -177,14 +210,14 @@ _PyImport_LoadDynamicModuleWithSpec(PyObject *spec, FILE *fp) "initialization of %s failed without raising an exception", name_buf); } - goto error; + return -1; } else if (PyErr_Occurred()) { _PyErr_FormatFromCause( PyExc_SystemError, "initialization of %s raised unreported exception", name_buf); m = NULL; - goto error; + return -1; } if (Py_IS_TYPE(m, NULL)) { /* This can happen when a PyModuleDef is returned without calling @@ -194,60 +227,101 @@ _PyImport_LoadDynamicModuleWithSpec(PyObject *spec, FILE *fp) "init function of %s returned uninitialized object", name_buf); m = NULL; /* prevent segfault in DECREF */ - goto error; + return -1; } + if (PyObject_TypeCheck(m, &PyModuleDef_Type)) { - Py_DECREF(name_unicode); - Py_DECREF(name); - Py_DECREF(path); - return PyModule_FromDefAndSpec((PyModuleDef*)m, spec); + /* multi-phase init */ + + def = (PyModuleDef *)m; + m = NULL; + + /* Run PyModule_FromDefAndSpec() to finish loading the module. */ } + else { + /* single-phase init (legacy) */ - /* Fall back to single-phase init mechanism */ + /* Remember pointer to module init function. */ + def = PyModule_GetDef(m); + if (def == NULL) { + PyErr_Format(PyExc_SystemError, + "initialization of %s did not return an extension " + "module", name_buf); + Py_DECREF(m); + return -1; + } + def->m_base.m_init = p0; - if (_PyImport_CheckSubinterpIncompatibleExtensionAllowed(name_buf) < 0) { - goto error; + if (info->hook_prefix == nonascii_prefix) { + /* don't allow legacy init for non-ASCII module names */ + PyErr_Format( + PyExc_SystemError, + "initialization of %s did not return PyModuleDef", + name_buf); + Py_DECREF(m); + return -1; + } + + /* Remember the filename as the __file__ attribute */ + if (PyModule_AddObjectRef(m, "__file__", info->path) < 0) { + PyErr_Clear(); /* Not important enough to report */ + } + + /* Run _PyImport_FixupExtensionObject() to finish loading the module. */ } - if (hook_prefix == nonascii_prefix) { - /* don't allow legacy init for non-ASCII module names */ - PyErr_Format( - PyExc_SystemError, - "initialization of %s did not return PyModuleDef", - name_buf); - goto error; + *res = (struct _Py_ext_module_loader_result){ + .def=def, + .module=m, + }; + return 0; +} + +PyObject * +_PyImport_LoadDynamicModuleWithSpec(PyObject *spec, FILE *fp) +{ + PyObject *m = NULL; + const char *name_buf; + //dl_funcptr exportfunc; + //PyModInitFunction p0; + PyModuleDef *def; + struct _Py_ext_module_loader_info info; + struct _Py_ext_module_loader_result res; + + if (_Py_ext_module_loader_info_from_spec(spec, &info) < 0) { + return NULL; } + name_buf = PyBytes_AS_STRING(info.name_encoded); - /* Remember pointer to module init function. */ - def = PyModule_GetDef(m); - if (def == NULL) { - PyErr_Format(PyExc_SystemError, - "initialization of %s did not return an extension " - "module", name_buf); - goto error; + if (_PyImport_RunDynamicModule(&info, fp, &res) < 0) { + _Py_ext_module_loader_info_clear(&info); + return NULL; } - def->m_base.m_init = p0; + m = res.module; + def = res.def; - /* Remember the filename as the __file__ attribute */ - if (PyModule_AddObjectRef(m, "__file__", path) < 0) { - PyErr_Clear(); /* Not important enough to report */ + if (m == NULL) { + _Py_ext_module_loader_info_clear(&info); + return PyModule_FromDefAndSpec(def, spec); } - PyObject *modules = PyImport_GetModuleDict(); - if (_PyImport_FixupExtensionObject(m, name_unicode, path, modules) < 0) + /* Fall back to single-phase init mechanism */ + + if (_PyImport_CheckSubinterpIncompatibleExtensionAllowed(name_buf) < 0) { goto error; + } - Py_DECREF(name_unicode); - Py_DECREF(name); - Py_DECREF(path); + PyObject *modules = PyImport_GetModuleDict(); + if (_PyImport_FixupExtensionObject(m, info.name, info.path, modules) < 0) { + goto error; + } + _Py_ext_module_loader_info_clear(&info); return m; error: - Py_DECREF(name_unicode); - Py_XDECREF(name); - Py_XDECREF(path); - Py_XDECREF(m); + _Py_ext_module_loader_info_clear(&info); + Py_DECREF(m); return NULL; } From 2e5ecde7e210eae8636f0b7300330eef2ccb96a4 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 7 Feb 2024 18:44:38 -0700 Subject: [PATCH 02/40] Drop _PyImport_LoadDynamicModuleWithSpec(). --- Include/internal/pycore_importdl.h | 2 - Python/import.c | 100 ++++++++++++++++++----------- Python/importdl.c | 48 -------------- 3 files changed, 61 insertions(+), 89 deletions(-) diff --git a/Include/internal/pycore_importdl.h b/Include/internal/pycore_importdl.h index 9683152736eb70..ce9c6366b510ad 100644 --- a/Include/internal/pycore_importdl.h +++ b/Include/internal/pycore_importdl.h @@ -37,8 +37,6 @@ extern int _PyImport_RunDynamicModule( FILE *fp, struct _Py_ext_module_loader_result *res); -extern PyObject *_PyImport_LoadDynamicModuleWithSpec(PyObject *spec, FILE *); - /* Max length of module suffix searched for -- accommodates "module.slb" */ #define MAXSUFFIXSIZE 12 diff --git a/Python/import.c b/Python/import.c index b040c7d5c0f7f5..03cbe19a1280b3 100644 --- a/Python/import.c +++ b/Python/import.c @@ -619,9 +619,9 @@ _PyImport_ClearModulesByIndex(PyInterpreterState *interp) (6). first time (not found in _PyRuntime.imports.extensions): 1. _imp_create_dynamic_impl() -> import_find_extension() - 2. _imp_create_dynamic_impl() -> _PyImport_LoadDynamicModuleWithSpec() - 3. _PyImport_LoadDynamicModuleWithSpec(): load - 4. _PyImport_LoadDynamicModuleWithSpec(): call + 2. _imp_create_dynamic_impl() -> _PyImport_RunDynamicModule() + 3. _PyImport_RunDynamicModule(): load + 4. _PyImport_RunDynamicModule(): call 5. -> PyModule_Create() -> PyModule_Create2() -> PyModule_CreateInitialized() 6. PyModule_CreateInitialized() -> PyModule_New() 7. PyModule_CreateInitialized(): allocate mod->md_state @@ -629,13 +629,13 @@ _PyImport_ClearModulesByIndex(PyInterpreterState *interp) 9. PyModule_CreateInitialized() -> PyModule_SetDocString() 10. PyModule_CreateInitialized(): set mod->md_def 11. : initialize the module - 12. _PyImport_LoadDynamicModuleWithSpec() -> _PyImport_CheckSubinterpIncompatibleExtensionAllowed() - 13. _PyImport_LoadDynamicModuleWithSpec(): set def->m_base.m_init - 14. _PyImport_LoadDynamicModuleWithSpec(): set __file__ - 15. _PyImport_LoadDynamicModuleWithSpec() -> _PyImport_FixupExtensionObject() - 16. _PyImport_FixupExtensionObject(): add it to interp->imports.modules_by_index - 17. _PyImport_FixupExtensionObject(): copy __dict__ into def->m_base.m_copy - 18. _PyImport_FixupExtensionObject(): add it to _PyRuntime.imports.extensions + 12. _PyImport_RunDynamicModule(): set def->m_base.m_init + 13. _PyImport_RunDynamicModule(): set __file__ + 14. _imp_create_dynamic_impl() -> _PyImport_CheckSubinterpIncompatibleExtensionAllowed() + 15. _imp_create_dynamic_impl() -> _PyImport_FixupExtensionObject() + 16. _PyImport_FixupExtensionObject(): add it to interp->imports.modules_by_index + 17. _PyImport_FixupExtensionObject(): copy __dict__ into def->m_base.m_copy + 18. _PyImport_FixupExtensionObject(): add it to _PyRuntime.imports.extensions (6). subsequent times (found in _PyRuntime.imports.extensions): 1. _imp_create_dynamic_impl() -> import_find_extension() @@ -658,7 +658,7 @@ _PyImport_ClearModulesByIndex(PyInterpreterState *interp) (6). main interpreter - first time (not found in _PyRuntime.imports.extensions): 1-16. (same as for m_size == -1) - 17. _PyImport_FixupExtensionObject(): add it to _PyRuntime.imports.extensions + 17. _PyImport_FixupExtensionObject(): add it to _PyRuntime.imports.extensions (6). previously loaded in main interpreter (found in _PyRuntime.imports.extensions): 1. _imp_create_dynamic_impl() -> import_find_extension() @@ -673,18 +673,18 @@ _PyImport_ClearModulesByIndex(PyInterpreterState *interp) (6). every time: 1. _imp_create_dynamic_impl() -> import_find_extension() (not found) - 2. _imp_create_dynamic_impl() -> _PyImport_LoadDynamicModuleWithSpec() - 3. _PyImport_LoadDynamicModuleWithSpec(): load module init func - 4. _PyImport_LoadDynamicModuleWithSpec(): call module init func - 5. _PyImport_LoadDynamicModuleWithSpec() -> PyModule_FromDefAndSpec() - 6. PyModule_FromDefAndSpec(): gather/check moduledef slots - 7. if there's a Py_mod_create slot: - 1. PyModule_FromDefAndSpec(): call its function - 8. else: - 1. PyModule_FromDefAndSpec() -> PyModule_NewObject() - 9: PyModule_FromDefAndSpec(): set mod->md_def - 10. PyModule_FromDefAndSpec() -> _add_methods_to_object() - 11. PyModule_FromDefAndSpec() -> PyModule_SetDocString() + 2. _imp_create_dynamic_impl() -> _PyImport_RunDynamicModule() + 3. _PyImport_RunDynamicModule(): load module init func + 4. _PyImport_RunDynamicModule(): call module init func + 5. _imp_create_dynamic_impl() -> PyModule_FromDefAndSpec() + 6. PyModule_FromDefAndSpec(): gather/check moduledef slots + 7. if there's a Py_mod_create slot: + 1. PyModule_FromDefAndSpec(): call its function + 8. else: + 1. PyModule_FromDefAndSpec() -> PyModule_NewObject() + 9: PyModule_FromDefAndSpec(): set mod->md_def + 10. PyModule_FromDefAndSpec() -> _add_methods_to_object() + 11. PyModule_FromDefAndSpec() -> PyModule_SetDocString() (10). every time: 1. _imp_exec_dynamic_impl() -> exec_builtin_or_dynamic() @@ -3724,44 +3724,66 @@ static PyObject * _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) /*[clinic end generated code: output=83249b827a4fde77 input=c31b954f4cf4e09d]*/ { - PyObject *mod, *name, *path; + PyObject *mod = NULL; FILE *fp; + struct _Py_ext_module_loader_info info; + struct _Py_ext_module_loader_result res; - name = PyObject_GetAttrString(spec, "name"); - if (name == NULL) { - return NULL; - } - - path = PyObject_GetAttrString(spec, "origin"); - if (path == NULL) { - Py_DECREF(name); + if (_Py_ext_module_loader_info_from_spec(spec, &info) < 0) { return NULL; } PyThreadState *tstate = _PyThreadState_GET(); - mod = import_find_extension(tstate, name, path); + mod = import_find_extension(tstate, info.name, info.path); if (mod != NULL || _PyErr_Occurred(tstate)) { assert(mod == NULL || !_PyErr_Occurred(tstate)); goto finally; } + /* Is multi-phase init or this is the first time being loaded. */ + if (file != NULL) { - fp = _Py_fopen_obj(path, "r"); + fp = _Py_fopen_obj(info.path, "r"); if (fp == NULL) { goto finally; } } - else + else { fp = NULL; + } + + if (_PyImport_RunDynamicModule(&info, fp, &res) < 0) { + goto finally; + } + mod = res.module; - mod = _PyImport_LoadDynamicModuleWithSpec(spec, fp); + if (mod == NULL) { + /* multi-phase init */ + + mod = PyModule_FromDefAndSpec(res.def, spec); + } + else { + /* Fall back to single-phase init mechanism */ + + const char *name_buf = PyBytes_AS_STRING(info.name_encoded); + if (_PyImport_CheckSubinterpIncompatibleExtensionAllowed(name_buf) < 0) { + Py_CLEAR(mod); + goto finally; + } - if (fp) + PyObject *modules = PyImport_GetModuleDict(); + if (_PyImport_FixupExtensionObject(mod, info.name, info.path, modules) < 0) { + Py_CLEAR(mod); + goto finally; + } + } + + if (fp) { fclose(fp); + } finally: - Py_DECREF(name); - Py_DECREF(path); + _Py_ext_module_loader_info_clear(&info); return mod; } diff --git a/Python/importdl.c b/Python/importdl.c index bb9b068e1cec1a..bbfb4620806b7e 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -277,52 +277,4 @@ _PyImport_RunDynamicModule(struct _Py_ext_module_loader_info *info, return 0; } -PyObject * -_PyImport_LoadDynamicModuleWithSpec(PyObject *spec, FILE *fp) -{ - PyObject *m = NULL; - const char *name_buf; - //dl_funcptr exportfunc; - //PyModInitFunction p0; - PyModuleDef *def; - struct _Py_ext_module_loader_info info; - struct _Py_ext_module_loader_result res; - - if (_Py_ext_module_loader_info_from_spec(spec, &info) < 0) { - return NULL; - } - name_buf = PyBytes_AS_STRING(info.name_encoded); - - if (_PyImport_RunDynamicModule(&info, fp, &res) < 0) { - _Py_ext_module_loader_info_clear(&info); - return NULL; - } - m = res.module; - def = res.def; - - if (m == NULL) { - _Py_ext_module_loader_info_clear(&info); - return PyModule_FromDefAndSpec(def, spec); - } - - /* Fall back to single-phase init mechanism */ - - if (_PyImport_CheckSubinterpIncompatibleExtensionAllowed(name_buf) < 0) { - goto error; - } - - PyObject *modules = PyImport_GetModuleDict(); - if (_PyImport_FixupExtensionObject(m, info.name, info.path, modules) < 0) { - goto error; - } - - _Py_ext_module_loader_info_clear(&info); - return m; - -error: - _Py_ext_module_loader_info_clear(&info); - Py_DECREF(m); - return NULL; -} - #endif /* HAVE_DYNAMIC_LOADING */ From b810cd8ae42ca37a0977d596f2fc75fa6243805f Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 7 Feb 2024 19:10:34 -0700 Subject: [PATCH 03/40] Add an assert. --- Python/import.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Python/import.c b/Python/import.c index 03cbe19a1280b3..76c28bdedbb380 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1202,6 +1202,7 @@ fix_up_extension(PyObject *mod, PyObject *name, PyObject *filename) // XXX Why special-case the main interpreter? if (_Py_IsMainInterpreter(tstate->interp) || def->m_size == -1) { + assert(_extensions_cache_get(filename, name) == NULL); if (_extensions_cache_set(filename, name, def) < 0) { return -1; } From 8d0ef38d53c565b17fa4d49a0be2ecf86edf85d3 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 7 Feb 2024 19:43:21 -0700 Subject: [PATCH 04/40] Factor out fix_up_extension_for_interpreter(). --- Include/internal/pycore_import.h | 1 + Python/import.c | 114 ++++++++++++++++++++----------- 2 files changed, 76 insertions(+), 39 deletions(-) diff --git a/Include/internal/pycore_import.h b/Include/internal/pycore_import.h index eb8a9a0db46c22..cb6c957c09fa7e 100644 --- a/Include/internal/pycore_import.h +++ b/Include/internal/pycore_import.h @@ -27,6 +27,7 @@ extern int _PyImport_FixupBuiltin( const char *name, /* UTF-8 encoded string */ PyObject *modules ); +// We could probably drop this: extern int _PyImport_FixupExtensionObject(PyObject*, PyObject *, PyObject *, PyObject *); diff --git a/Python/import.c b/Python/import.c index 76c28bdedbb380..4553b80cf40824 100644 --- a/Python/import.c +++ b/Python/import.c @@ -200,16 +200,22 @@ _PyImport_ClearModules(PyInterpreterState *interp) Py_SETREF(MODULES(interp), NULL); } -PyObject * -PyImport_GetModuleDict(void) +static inline PyObject * +get_modules_dict(PyInterpreterState *interp) { - PyInterpreterState *interp = _PyInterpreterState_GET(); if (MODULES(interp) == NULL) { Py_FatalError("interpreter has no modules dictionary"); } return MODULES(interp); } +PyObject * +PyImport_GetModuleDict(void) +{ + PyInterpreterState *interp = _PyInterpreterState_GET(); + return get_modules_dict(interp); +} + int _PyImport_SetModule(PyObject *name, PyObject *m) { @@ -632,10 +638,12 @@ _PyImport_ClearModulesByIndex(PyInterpreterState *interp) 12. _PyImport_RunDynamicModule(): set def->m_base.m_init 13. _PyImport_RunDynamicModule(): set __file__ 14. _imp_create_dynamic_impl() -> _PyImport_CheckSubinterpIncompatibleExtensionAllowed() - 15. _imp_create_dynamic_impl() -> _PyImport_FixupExtensionObject() - 16. _PyImport_FixupExtensionObject(): add it to interp->imports.modules_by_index - 17. _PyImport_FixupExtensionObject(): copy __dict__ into def->m_base.m_copy - 18. _PyImport_FixupExtensionObject(): add it to _PyRuntime.imports.extensions + 15. _imp_create_dynamic_impl() -> fix_up_extension() + 16. fix_up_extension() -> fix_up_extension_for_interpreter() + 17. fix_up_extension_for_interpreter(): set it on sys.modules + 18. fix_up_extension_for_interpreter(): add it to interp->imports.modules_by_index + 19. fix_up_extension_for_interpreter(): copy __dict__ into def->m_base.m_copy + 20. fix_up_extension(): add it to _PyRuntime.imports.extensions (6). subsequent times (found in _PyRuntime.imports.extensions): 1. _imp_create_dynamic_impl() -> import_find_extension() @@ -657,8 +665,8 @@ _PyImport_ClearModulesByIndex(PyInterpreterState *interp) 1-16. (same as for m_size == -1) (6). main interpreter - first time (not found in _PyRuntime.imports.extensions): - 1-16. (same as for m_size == -1) - 17. _PyImport_FixupExtensionObject(): add it to _PyRuntime.imports.extensions + 1-19. (same as for m_size == -1) + 20. fix_up_extension(): add it to _PyRuntime.imports.extensions (6). previously loaded in main interpreter (found in _PyRuntime.imports.extensions): 1. _imp_create_dynamic_impl() -> import_find_extension() @@ -894,7 +902,7 @@ extensions_lock_release(void) (module name, module name) (for built-in modules) or by (filename, module name) (for dynamically loaded modules), containing these modules. A copy of the module's dictionary is stored by calling - _PyImport_FixupExtensionObject() immediately after the module initialization + fix_up_extension() immediately after the module initialization function succeeds. A copy can be retrieved from there by calling import_find_extension(). @@ -1159,28 +1167,29 @@ is_core_module(PyInterpreterState *interp, PyObject *name, PyObject *filename) } static int -fix_up_extension(PyObject *mod, PyObject *name, PyObject *filename) +fix_up_extension_for_interpreter(PyInterpreterState *interp, + PyObject *mod, PyModuleDef *def, + PyObject *name, PyObject *filename, + PyObject *modules) { - if (mod == NULL || !PyModule_Check(mod)) { - PyErr_BadInternalCall(); - return -1; - } + assert(mod != NULL && PyModule_Check(mod)); + assert(def == PyModule_GetDef(mod)); - struct PyModuleDef *def = PyModule_GetDef(mod); - if (!def) { - PyErr_BadInternalCall(); + if (modules == NULL) { + modules = get_modules_dict(interp); + } + if (PyObject_SetItem(modules, name, mod) < 0) { return -1; } - PyThreadState *tstate = _PyThreadState_GET(); - if (_modules_by_index_set(tstate->interp, def, mod) < 0) { - return -1; + if (_modules_by_index_set(interp, def, mod) < 0) { + goto error; } // bpo-44050: Extensions and def->m_base.m_copy can be updated // when the extension module doesn't support sub-interpreters. if (def->m_size == -1) { - if (!is_core_module(tstate->interp, name, filename)) { + if (!is_core_module(interp, name, filename)) { assert(PyUnicode_CompareWithASCIIString(name, "sys") != 0); assert(PyUnicode_CompareWithASCIIString(name, "builtins") != 0); if (def->m_base.m_copy) { @@ -1191,18 +1200,49 @@ fix_up_extension(PyObject *mod, PyObject *name, PyObject *filename) } PyObject *dict = PyModule_GetDict(mod); if (dict == NULL) { - return -1; + goto error; } def->m_base.m_copy = PyDict_Copy(dict); if (def->m_base.m_copy == NULL) { - return -1; + goto error; } } } + return 0; + +error: + PyMapping_DelItem(modules, name); + return -1; +} + + +static int +fix_up_extension(PyObject *mod, PyModuleDef *def, + PyObject *name, PyObject *filename, + PyObject *modules) +{ + PyInterpreterState *interp = _PyInterpreterState_GET(); + if (def == NULL) { + def = PyModule_GetDef(mod); + if (def == NULL) { + PyErr_BadInternalCall(); + return -1; + } + } + + if (fix_up_extension_for_interpreter( + interp, mod, def, name, filename, modules) < 0) + { + return -1; + } + // XXX Why special-case the main interpreter? - if (_Py_IsMainInterpreter(tstate->interp) || def->m_size == -1) { - assert(_extensions_cache_get(filename, name) == NULL); + if (_Py_IsMainInterpreter(interp) || def->m_size == -1) { +#ifndef NDEBUG + PyModuleDef *cached = _extensions_cache_get(filename, name); + assert(cached == NULL || cached == def); +#endif if (_extensions_cache_set(filename, name, def) < 0) { return -1; } @@ -1215,11 +1255,11 @@ int _PyImport_FixupExtensionObject(PyObject *mod, PyObject *name, PyObject *filename, PyObject *modules) { - if (PyObject_SetItem(modules, name, mod) < 0) { + if (mod == NULL || !PyModule_Check(mod)) { + PyErr_BadInternalCall(); return -1; } - if (fix_up_extension(mod, name, filename) < 0) { - PyMapping_DelItem(modules, name); + if (fix_up_extension(mod, NULL, name, filename, modules) < 0) { return -1; } return 0; @@ -1342,11 +1382,8 @@ _PyImport_FixupBuiltin(PyObject *mod, const char *name, PyObject *modules) if (nameobj == NULL) { return -1; } - if (PyObject_SetItem(modules, nameobj, mod) < 0) { - goto finally; - } - if (fix_up_extension(mod, nameobj, nameobj) < 0) { - PyMapping_DelItem(modules, nameobj); + assert(mod != NULL && PyModule_Check(mod)); + if (fix_up_extension(mod, NULL, nameobj, nameobj, modules) < 0) { goto finally; } res = 0; @@ -1398,15 +1435,15 @@ create_builtin(PyThreadState *tstate, PyObject *name, PyObject *spec) return PyModule_FromDefAndSpec((PyModuleDef*)mod, spec); } else { + assert(PyModule_Check(mod)); /* Remember pointer to module init function. */ PyModuleDef *def = PyModule_GetDef(mod); if (def == NULL) { return NULL; } - def->m_base.m_init = p->initfunc; - if (_PyImport_FixupExtensionObject(mod, name, name, - modules) < 0) { + + if (fix_up_extension(mod, def, name, name, modules) < 0) { return NULL; } return mod; @@ -3772,8 +3809,7 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) goto finally; } - PyObject *modules = PyImport_GetModuleDict(); - if (_PyImport_FixupExtensionObject(mod, info.name, info.path, modules) < 0) { + if (fix_up_extension(mod, res.def, info.name, info.path, NULL) < 0) { Py_CLEAR(mod); goto finally; } From 7f930ccdfdb720fc344a55ab5ce1523e46027e4f Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 7 Feb 2024 19:57:37 -0700 Subject: [PATCH 05/40] Do _extensions_cache_set() first. --- Python/import.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/Python/import.c b/Python/import.c index 4553b80cf40824..9ba9ac9033aaac 100644 --- a/Python/import.c +++ b/Python/import.c @@ -639,11 +639,11 @@ _PyImport_ClearModulesByIndex(PyInterpreterState *interp) 13. _PyImport_RunDynamicModule(): set __file__ 14. _imp_create_dynamic_impl() -> _PyImport_CheckSubinterpIncompatibleExtensionAllowed() 15. _imp_create_dynamic_impl() -> fix_up_extension() - 16. fix_up_extension() -> fix_up_extension_for_interpreter() - 17. fix_up_extension_for_interpreter(): set it on sys.modules - 18. fix_up_extension_for_interpreter(): add it to interp->imports.modules_by_index - 19. fix_up_extension_for_interpreter(): copy __dict__ into def->m_base.m_copy - 20. fix_up_extension(): add it to _PyRuntime.imports.extensions + 16. fix_up_extension(): add it to _PyRuntime.imports.extensions + 17. fix_up_extension() -> fix_up_extension_for_interpreter() + 18. fix_up_extension_for_interpreter(): set it on sys.modules + 19. fix_up_extension_for_interpreter(): add it to interp->imports.modules_by_index + 20. fix_up_extension_for_interpreter(): copy __dict__ into def->m_base.m_copy (6). subsequent times (found in _PyRuntime.imports.extensions): 1. _imp_create_dynamic_impl() -> import_find_extension() @@ -662,11 +662,12 @@ _PyImport_ClearModulesByIndex(PyInterpreterState *interp) ...for single-phase init modules, where m_size >= 0: (6). not main interpreter and never loaded there - every time (not found in _PyRuntime.imports.extensions): - 1-16. (same as for m_size == -1) + 1-20. (same as for m_size == -1) (6). main interpreter - first time (not found in _PyRuntime.imports.extensions): - 1-19. (same as for m_size == -1) - 20. fix_up_extension(): add it to _PyRuntime.imports.extensions + 1-15. (same as for m_size == -1) + 16. fix_up_extension(): add it to _PyRuntime.imports.extensions + 17-20. (same as for m_size == -1) (6). previously loaded in main interpreter (found in _PyRuntime.imports.extensions): 1. _imp_create_dynamic_impl() -> import_find_extension() @@ -1231,12 +1232,6 @@ fix_up_extension(PyObject *mod, PyModuleDef *def, } } - if (fix_up_extension_for_interpreter( - interp, mod, def, name, filename, modules) < 0) - { - return -1; - } - // XXX Why special-case the main interpreter? if (_Py_IsMainInterpreter(interp) || def->m_size == -1) { #ifndef NDEBUG @@ -1248,6 +1243,12 @@ fix_up_extension(PyObject *mod, PyModuleDef *def, } } + if (fix_up_extension_for_interpreter( + interp, mod, def, name, filename, modules) < 0) + { + return -1; + } + return 0; } From 1ffeaef7f8f1ed1082de6cc5170ce3f3733b3965 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 16 Apr 2024 12:27:20 -0600 Subject: [PATCH 06/40] Calculate path_bytes ahead of time. --- Include/internal/pycore_importdl.h | 3 ++ Python/importdl.c | 59 ++++++++++++++---------------- 2 files changed, 31 insertions(+), 31 deletions(-) diff --git a/Include/internal/pycore_importdl.h b/Include/internal/pycore_importdl.h index ce9c6366b510ad..71e115ba8ad35a 100644 --- a/Include/internal/pycore_importdl.h +++ b/Include/internal/pycore_importdl.h @@ -18,6 +18,9 @@ typedef PyObject *(*PyModInitFunction)(void); struct _Py_ext_module_loader_info { PyObject *path; +#ifndef MS_WINDOWS + PyObject *path_encoded; +#endif PyObject *name; PyObject *name_encoded; const char *hook_prefix; diff --git a/Python/importdl.c b/Python/importdl.c index bbfb4620806b7e..1b50c89a8d7560 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -97,47 +97,51 @@ void _Py_ext_module_loader_info_clear(struct _Py_ext_module_loader_info *info) { Py_CLEAR(info->path); +#ifndef MS_WINDOWS + Py_CLEAR(info->path_encoded); +#endif Py_CLEAR(info->name); Py_CLEAR(info->name_encoded); } int _Py_ext_module_loader_info_from_spec(PyObject *spec, - struct _Py_ext_module_loader_info *info) + struct _Py_ext_module_loader_info *p_info) { - PyObject *name_unicode = NULL, *name = NULL, *path = NULL; - const char *hook_prefix; + struct _Py_ext_module_loader_info info = {0}; - name_unicode = PyObject_GetAttrString(spec, "name"); - if (name_unicode == NULL) { + info.name = PyObject_GetAttrString(spec, "name"); + if (info.name == NULL) { return -1; } - if (!PyUnicode_Check(name_unicode)) { + if (!PyUnicode_Check(info.name)) { PyErr_SetString(PyExc_TypeError, "spec.name must be a string"); - Py_DECREF(name_unicode); + _Py_ext_module_loader_info_clear(&info); return -1; } - name = get_encoded_name(name_unicode, &hook_prefix); - if (name == NULL) { - Py_DECREF(name_unicode); + info.name_encoded = get_encoded_name(info.name, &info.hook_prefix); + if (info.name_encoded == NULL) { + _Py_ext_module_loader_info_clear(&info); return -1; } - path = PyObject_GetAttrString(spec, "origin"); - if (path == NULL) { - Py_DECREF(name_unicode); - Py_DECREF(name); + info.path = PyObject_GetAttrString(spec, "origin"); + if (info.path == NULL) { + _Py_ext_module_loader_info_clear(&info); return -1; } - *info = (struct _Py_ext_module_loader_info){ - .path=path, - .name=name_unicode, - .name_encoded=name, - .hook_prefix=hook_prefix, - }; +#ifndef MS_WINDOWS + info.path_encoded = PyUnicode_EncodeFSDefault(info.path); + if (info.path_encoded == NULL) { + _Py_ext_module_loader_info_clear(&info); + return -1; + } +#endif + + *p_info = info; return 0; } @@ -146,10 +150,6 @@ _PyImport_RunDynamicModule(struct _Py_ext_module_loader_info *info, FILE *fp, struct _Py_ext_module_loader_result *res) { -#ifndef MS_WINDOWS - PyObject *pathbytes = NULL; - const char *path_buf; -#endif PyObject *m = NULL; const char *name_buf = PyBytes_AS_STRING(info->name_encoded); const char *oldcontext, *newcontext; @@ -171,14 +171,11 @@ _PyImport_RunDynamicModule(struct _Py_ext_module_loader_info *info, exportfunc = _PyImport_FindSharedFuncptrWindows( info->hook_prefix, name_buf, info->path, fp); #else - pathbytes = PyUnicode_EncodeFSDefault(info->path); - if (pathbytes == NULL) { - return -1; + { + const char *path_buf = PyBytes_AS_STRING(info->path_encoded); + exportfunc = _PyImport_FindSharedFuncptr( + info->hook_prefix, name_buf, path_buf, fp); } - path_buf = PyBytes_AS_STRING(pathbytes); - exportfunc = _PyImport_FindSharedFuncptr( - info->hook_prefix, name_buf, path_buf, fp); - Py_DECREF(pathbytes); #endif if (exportfunc == NULL) { From 7500285cbd722de6670524dbcc01f3d534aeb036 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 16 Apr 2024 12:38:25 -0600 Subject: [PATCH 07/40] Move the audit entry to _imp_create_dynamic_impl(). --- Python/import.c | 6 ++++++ Python/importdl.c | 5 ----- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/Python/import.c b/Python/import.c index 9ba9ac9033aaac..5bd6a16668957e 100644 --- a/Python/import.c +++ b/Python/import.c @@ -3779,6 +3779,12 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) goto finally; } + if (PySys_Audit("import", "OOOOO", info.name, info.path, + Py_None, Py_None, Py_None) < 0) + { + goto finally; + } + /* Is multi-phase init or this is the first time being loaded. */ if (file != NULL) { diff --git a/Python/importdl.c b/Python/importdl.c index 1b50c89a8d7560..991522bb292e2e 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -162,11 +162,6 @@ _PyImport_RunDynamicModule(struct _Py_ext_module_loader_info *info, return -1; } - if (PySys_Audit("import", "OOOOO", info->name, info->path, - Py_None, Py_None, Py_None) < 0) { - return -1; - } - #ifdef MS_WINDOWS exportfunc = _PyImport_FindSharedFuncptrWindows( info->hook_prefix, name_buf, info->path, fp); From 3c0b1700ad0bf546d27df8637887360d9e37743c Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 16 Apr 2024 13:07:30 -0600 Subject: [PATCH 08/40] _Py_ext_module_loader_info_from_spec() -> _Py_ext_module_loader_info_init_from_spec() --- Include/internal/pycore_importdl.h | 6 +++--- Python/import.c | 2 +- Python/importdl.c | 5 +++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/Include/internal/pycore_importdl.h b/Include/internal/pycore_importdl.h index 71e115ba8ad35a..7c865f73ed8620 100644 --- a/Include/internal/pycore_importdl.h +++ b/Include/internal/pycore_importdl.h @@ -27,9 +27,9 @@ struct _Py_ext_module_loader_info { }; extern void _Py_ext_module_loader_info_clear( struct _Py_ext_module_loader_info *info); -extern int _Py_ext_module_loader_info_from_spec( - PyObject *spec, - struct _Py_ext_module_loader_info *info); +extern int _Py_ext_module_loader_info_init_from_spec( + struct _Py_ext_module_loader_info *info, + PyObject *spec); struct _Py_ext_module_loader_result { PyModuleDef *def; diff --git a/Python/import.c b/Python/import.c index 5bd6a16668957e..f941ab9b6639f7 100644 --- a/Python/import.c +++ b/Python/import.c @@ -3768,7 +3768,7 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) struct _Py_ext_module_loader_info info; struct _Py_ext_module_loader_result res; - if (_Py_ext_module_loader_info_from_spec(spec, &info) < 0) { + if (_Py_ext_module_loader_info_init_from_spec(&info, spec) < 0) { return NULL; } diff --git a/Python/importdl.c b/Python/importdl.c index 991522bb292e2e..448232e3aa631f 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -105,8 +105,9 @@ _Py_ext_module_loader_info_clear(struct _Py_ext_module_loader_info *info) } int -_Py_ext_module_loader_info_from_spec(PyObject *spec, - struct _Py_ext_module_loader_info *p_info) +_Py_ext_module_loader_info_init_from_spec( + struct _Py_ext_module_loader_info *p_info, + PyObject *spec) { struct _Py_ext_module_loader_info info = {0}; From 02e24ccd22fe609b2d45ba4a1ab2ee128c7337b3 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 16 Apr 2024 14:17:03 -0600 Subject: [PATCH 09/40] Add info.newcontext. --- Include/internal/pycore_importdl.h | 1 + Python/importdl.c | 15 ++++++++------- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/Include/internal/pycore_importdl.h b/Include/internal/pycore_importdl.h index 7c865f73ed8620..7cb876b5996bba 100644 --- a/Include/internal/pycore_importdl.h +++ b/Include/internal/pycore_importdl.h @@ -24,6 +24,7 @@ struct _Py_ext_module_loader_info { PyObject *name; PyObject *name_encoded; const char *hook_prefix; + const char *newcontext; }; extern void _Py_ext_module_loader_info_clear( struct _Py_ext_module_loader_info *info); diff --git a/Python/importdl.c b/Python/importdl.c index 448232e3aa631f..26bc8f056fe30e 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -128,6 +128,12 @@ _Py_ext_module_loader_info_init_from_spec( return -1; } + info.newcontext = PyUnicode_AsUTF8(info.name); + if (info.newcontext == NULL) { + _Py_ext_module_loader_info_clear(&info); + return -1; + } + info.path = PyObject_GetAttrString(spec, "origin"); if (info.path == NULL) { _Py_ext_module_loader_info_clear(&info); @@ -153,16 +159,11 @@ _PyImport_RunDynamicModule(struct _Py_ext_module_loader_info *info, { PyObject *m = NULL; const char *name_buf = PyBytes_AS_STRING(info->name_encoded); - const char *oldcontext, *newcontext; + const char *oldcontext; dl_funcptr exportfunc; PyModInitFunction p0; PyModuleDef *def; - newcontext = PyUnicode_AsUTF8(info->name); - if (newcontext == NULL) { - return -1; - } - #ifdef MS_WINDOWS exportfunc = _PyImport_FindSharedFuncptrWindows( info->hook_prefix, name_buf, info->path, fp); @@ -192,7 +193,7 @@ _PyImport_RunDynamicModule(struct _Py_ext_module_loader_info *info, p0 = (PyModInitFunction)exportfunc; /* Package context is needed for single-phase init */ - oldcontext = _PyImport_SwapPackageContext(newcontext); + oldcontext = _PyImport_SwapPackageContext(info->newcontext); m = p0(); _PyImport_SwapPackageContext(oldcontext); From ba5ffa78bc983ccc06bb32ea16c42aaa51143da0 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 16 Apr 2024 14:52:27 -0600 Subject: [PATCH 10/40] Split up _PyImport_RunDynamicModule() along cross-interpreter-safe boundaries. --- Include/internal/pycore_importdl.h | 2 + Python/importdl.c | 153 ++++++++++++++++++++--------- 2 files changed, 106 insertions(+), 49 deletions(-) diff --git a/Include/internal/pycore_importdl.h b/Include/internal/pycore_importdl.h index 7cb876b5996bba..1099540fdeafd4 100644 --- a/Include/internal/pycore_importdl.h +++ b/Include/internal/pycore_importdl.h @@ -35,6 +35,8 @@ extern int _Py_ext_module_loader_info_init_from_spec( struct _Py_ext_module_loader_result { PyModuleDef *def; PyObject *module; + int singlephase; + char err[200]; }; extern int _PyImport_RunDynamicModule( struct _Py_ext_module_loader_info *info, diff --git a/Python/importdl.c b/Python/importdl.c index 26bc8f056fe30e..dbdebf04c2b355 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -152,18 +152,29 @@ _Py_ext_module_loader_info_init_from_spec( return 0; } -int -_PyImport_RunDynamicModule(struct _Py_ext_module_loader_info *info, - FILE *fp, - struct _Py_ext_module_loader_result *res) +static void +_Py_ext_module_loader_result_apply_error( + struct _Py_ext_module_loader_result *res) +{ + if (res->err != NULL) { + if (PyErr_Occurred()) { + _PyErr_FormatFromCause(PyExc_SystemError, res->err); + } + else { + PyErr_SetString(PyExc_SystemError, res->err); + } + } + else { + assert(PyErr_Occurred()); + } +} + +static PyModInitFunction +_PyImport_GetModInitFunc(struct _Py_ext_module_loader_info *info, + FILE *fp) { - PyObject *m = NULL; const char *name_buf = PyBytes_AS_STRING(info->name_encoded); - const char *oldcontext; dl_funcptr exportfunc; - PyModInitFunction p0; - PyModuleDef *def; - #ifdef MS_WINDOWS exportfunc = _PyImport_FindSharedFuncptrWindows( info->hook_prefix, name_buf, info->path, fp); @@ -187,87 +198,131 @@ _PyImport_RunDynamicModule(struct _Py_ext_module_loader_info *info, Py_DECREF(msg); } } - return -1; + return NULL; } - p0 = (PyModInitFunction)exportfunc; + return (PyModInitFunction)exportfunc; +} + +static int +_PyImport_RunModInitFunc(PyModInitFunction p0, + struct _Py_ext_module_loader_info *info, + struct _Py_ext_module_loader_result *p_res) +{ + struct _Py_ext_module_loader_result res = { + .singlephase=-1, + }; + const char *name_buf = PyBytes_AS_STRING(info->name_encoded); /* Package context is needed for single-phase init */ - oldcontext = _PyImport_SwapPackageContext(info->newcontext); - m = p0(); + const char *oldcontext = _PyImport_SwapPackageContext(info->newcontext); + PyObject *m = p0(); _PyImport_SwapPackageContext(oldcontext); +#ifdef NDEBUG +# define SET_ERROR(...) \ + (void)snprintf(res.err, Py_ARRAY_LENGTH(res.err), __VA_ARGS__) +#else +# define SET_ERROR(...) \ + do { \ + int n = snprintf(res.err, Py_ARRAY_LENGTH(res.err), __VA_ARGS__); \ + assert(n < Py_ARRAY_LENGTH(res.err)); \ + } while (0) +#endif + if (m == NULL) { if (!PyErr_Occurred()) { - PyErr_Format( - PyExc_SystemError, + SET_ERROR( "initialization of %s failed without raising an exception", name_buf); } - return -1; + goto error; } else if (PyErr_Occurred()) { - _PyErr_FormatFromCause( - PyExc_SystemError, - "initialization of %s raised unreported exception", - name_buf); + SET_ERROR("initialization of %s raised unreported exception", + name_buf); + /* We would probably be correct to decref m here, + * but we weren't doing so before, + * so we stick with doing nothing. */ m = NULL; - return -1; + goto error; } + if (Py_IS_TYPE(m, NULL)) { /* This can happen when a PyModuleDef is returned without calling * PyModuleDef_Init on it */ - PyErr_Format(PyExc_SystemError, - "init function of %s returned uninitialized object", - name_buf); + SET_ERROR("init function of %s returned uninitialized object", + name_buf); + /* Likewise, decref'ing here makes sense. However, the original + * code has a note about "prevent segfault in DECREF", + * so we play it safe and leave it alone. */ m = NULL; /* prevent segfault in DECREF */ - return -1; + goto error; } if (PyObject_TypeCheck(m, &PyModuleDef_Type)) { /* multi-phase init */ - - def = (PyModuleDef *)m; - m = NULL; - - /* Run PyModule_FromDefAndSpec() to finish loading the module. */ + res.singlephase = 0; + res.def = (PyModuleDef *)m; } else { /* single-phase init (legacy) */ + res.singlephase = 1; + res.module = m; /* Remember pointer to module init function. */ - def = PyModule_GetDef(m); - if (def == NULL) { - PyErr_Format(PyExc_SystemError, - "initialization of %s did not return an extension " - "module", name_buf); - Py_DECREF(m); - return -1; + res.def = PyModule_GetDef(m); + if (res.def == NULL) { + SET_ERROR("initialization of %s did not return an extension " + "module", name_buf); + goto error; } - def->m_base.m_init = p0; + res.def->m_base.m_init = p0; if (info->hook_prefix == nonascii_prefix) { /* don't allow legacy init for non-ASCII module names */ - PyErr_Format( - PyExc_SystemError, - "initialization of %s did not return PyModuleDef", - name_buf); - Py_DECREF(m); - return -1; + SET_ERROR("initialization of %s did not return PyModuleDef", + name_buf); + goto error; } + } +#undef SET_ERROR + *p_res = res; + return 0; + +error: + Py_CLEAR(res.module); + res.def = NULL; + *p_res = res; + return -1; +} + +int +_PyImport_RunDynamicModule(struct _Py_ext_module_loader_info *info, + FILE *fp, + struct _Py_ext_module_loader_result *res) +{ + PyModInitFunction p0 = _PyImport_GetModInitFunc(info, fp); + if (p0 == NULL) { + return -1; + } + + if (_PyImport_RunModInitFunc(p0, info, res) < 0) { + _Py_ext_module_loader_result_apply_error(res); + return -1; + } + + if (res->singlephase) { /* Remember the filename as the __file__ attribute */ - if (PyModule_AddObjectRef(m, "__file__", info->path) < 0) { + if (PyModule_AddObjectRef(res->module, "__file__", info->path) < 0) { PyErr_Clear(); /* Not important enough to report */ } /* Run _PyImport_FixupExtensionObject() to finish loading the module. */ } + /* else: Run PyModule_FromDefAndSpec() to finish loading the module. */ - *res = (struct _Py_ext_module_loader_result){ - .def=def, - .module=m, - }; return 0; } From b89596625b1977f59a3cd7c08ac0f7988c1c04d4 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 16 Apr 2024 15:04:56 -0600 Subject: [PATCH 11/40] Drop _PyImport_RunDynamicModule(). --- Include/internal/pycore_importdl.h | 11 +++++++--- Python/import.c | 27 +++++++++++++++++------- Python/importdl.c | 34 +++--------------------------- 3 files changed, 30 insertions(+), 42 deletions(-) diff --git a/Include/internal/pycore_importdl.h b/Include/internal/pycore_importdl.h index 1099540fdeafd4..4f06c27280698e 100644 --- a/Include/internal/pycore_importdl.h +++ b/Include/internal/pycore_importdl.h @@ -38,10 +38,15 @@ struct _Py_ext_module_loader_result { int singlephase; char err[200]; }; -extern int _PyImport_RunDynamicModule( - struct _Py_ext_module_loader_info *info, - FILE *fp, +extern void _Py_ext_module_loader_result_apply_error( struct _Py_ext_module_loader_result *res); +extern PyModInitFunction _PyImport_GetModInitFunc( + struct _Py_ext_module_loader_info *info, + FILE *fp); +extern int _PyImport_RunModInitFunc( + PyModInitFunction p0, + struct _Py_ext_module_loader_info *info, + struct _Py_ext_module_loader_result *p_res); /* Max length of module suffix searched for -- accommodates "module.slb" */ #define MAXSUFFIXSIZE 12 diff --git a/Python/import.c b/Python/import.c index f941ab9b6639f7..1dc9b1cccc03a8 100644 --- a/Python/import.c +++ b/Python/import.c @@ -3787,6 +3787,9 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) /* Is multi-phase init or this is the first time being loaded. */ + /* We would move this (and the fclose() below) into + * _PyImport_GetModInitFunc(), but it isn't clear if the intervening + * code relies on fp still being open. */ if (file != NULL) { fp = _Py_fopen_obj(info.path, "r"); if (fp == NULL) { @@ -3797,18 +3800,22 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) fp = NULL; } - if (_PyImport_RunDynamicModule(&info, fp, &res) < 0) { + PyModInitFunction p0 = _PyImport_GetModInitFunc(&info, fp); + if (p0 == NULL) { goto finally; } - mod = res.module; + if (_PyImport_RunModInitFunc(p0, &info, &res) < 0) { + _Py_ext_module_loader_result_apply_error(&res); + return NULL; + } - if (mod == NULL) { - /* multi-phase init */ + if (res.singlephase) { + mod = Py_NewRef(res.module); - mod = PyModule_FromDefAndSpec(res.def, spec); - } - else { - /* Fall back to single-phase init mechanism */ + /* Remember the filename as the __file__ attribute */ + if (PyModule_AddObjectRef(mod, "__file__", info.path) < 0) { + PyErr_Clear(); /* Not important enough to report */ + } const char *name_buf = PyBytes_AS_STRING(info.name_encoded); if (_PyImport_CheckSubinterpIncompatibleExtensionAllowed(name_buf) < 0) { @@ -3821,7 +3828,11 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) goto finally; } } + else { + mod = PyModule_FromDefAndSpec(res.def, spec); + } + // XXX This should happen in the error cases too. if (fp) { fclose(fp); } diff --git a/Python/importdl.c b/Python/importdl.c index dbdebf04c2b355..c20a7c54b7fbe9 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -152,7 +152,7 @@ _Py_ext_module_loader_info_init_from_spec( return 0; } -static void +void _Py_ext_module_loader_result_apply_error( struct _Py_ext_module_loader_result *res) { @@ -169,7 +169,7 @@ _Py_ext_module_loader_result_apply_error( } } -static PyModInitFunction +PyModInitFunction _PyImport_GetModInitFunc(struct _Py_ext_module_loader_info *info, FILE *fp) { @@ -204,7 +204,7 @@ _PyImport_GetModInitFunc(struct _Py_ext_module_loader_info *info, return (PyModInitFunction)exportfunc; } -static int +int _PyImport_RunModInitFunc(PyModInitFunction p0, struct _Py_ext_module_loader_info *info, struct _Py_ext_module_loader_result *p_res) @@ -298,32 +298,4 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, return -1; } -int -_PyImport_RunDynamicModule(struct _Py_ext_module_loader_info *info, - FILE *fp, - struct _Py_ext_module_loader_result *res) -{ - PyModInitFunction p0 = _PyImport_GetModInitFunc(info, fp); - if (p0 == NULL) { - return -1; - } - - if (_PyImport_RunModInitFunc(p0, info, res) < 0) { - _Py_ext_module_loader_result_apply_error(res); - return -1; - } - - if (res->singlephase) { - /* Remember the filename as the __file__ attribute */ - if (PyModule_AddObjectRef(res->module, "__file__", info->path) < 0) { - PyErr_Clear(); /* Not important enough to report */ - } - - /* Run _PyImport_FixupExtensionObject() to finish loading the module. */ - } - /* else: Run PyModule_FromDefAndSpec() to finish loading the module. */ - - return 0; -} - #endif /* HAVE_DYNAMIC_LOADING */ From 76fa2735568194838576659d0a39008df8c7d374 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 16 Apr 2024 16:31:28 -0600 Subject: [PATCH 12/40] Always close the file handle. --- Python/import.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Python/import.c b/Python/import.c index 1dc9b1cccc03a8..d69d9dd3781996 100644 --- a/Python/import.c +++ b/Python/import.c @@ -3832,12 +3832,10 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) mod = PyModule_FromDefAndSpec(res.def, spec); } - // XXX This should happen in the error cases too. +finally: if (fp) { fclose(fp); } - -finally: _Py_ext_module_loader_info_clear(&info); return mod; } From 1e936c5084fc61e06d14b9971d8b5f84cfb8a792 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 16 Apr 2024 16:31:28 -0600 Subject: [PATCH 13/40] Revert the fclose() change. --- Python/import.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Python/import.c b/Python/import.c index d69d9dd3781996..ae8c4496a13014 100644 --- a/Python/import.c +++ b/Python/import.c @@ -3832,10 +3832,12 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) mod = PyModule_FromDefAndSpec(res.def, spec); } -finally: + // XXX Shouldn't this happen in the error cases too. if (fp) { fclose(fp); } + +finally: _Py_ext_module_loader_info_clear(&info); return mod; } From 6bb07b94720fa4479042a1a274ae8a40875091de Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 16 Apr 2024 16:49:34 -0600 Subject: [PATCH 14/40] Factor out create_dynamic(). --- Python/import.c | 114 +++++++++++++++++++++++++++--------------------- 1 file changed, 64 insertions(+), 50 deletions(-) diff --git a/Python/import.c b/Python/import.c index ae8c4496a13014..bc4dd0faa887ae 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1369,6 +1369,68 @@ clear_singlephase_extension(PyInterpreterState *interp, return 0; } +static PyObject * +create_dynamic(struct _Py_ext_module_loader_info *info, PyObject *file, + PyObject *spec) +{ + FILE *fp; + struct _Py_ext_module_loader_result res; + PyObject *mod = NULL; + + /* We would move this (and the fclose() below) into + * _PyImport_GetModInitFunc(), but it isn't clear if the intervening + * code relies on fp still being open. */ + if (file != NULL) { + fp = _Py_fopen_obj(info->path, "r"); + if (fp == NULL) { + goto finally; + } + } + else { + fp = NULL; + } + + PyModInitFunction p0 = _PyImport_GetModInitFunc(info, fp); + if (p0 == NULL) { + goto finally; + } + if (_PyImport_RunModInitFunc(p0, info, &res) < 0) { + _Py_ext_module_loader_result_apply_error(&res); + goto finally; + } + + if (res.singlephase) { + mod = Py_NewRef(res.module); + + /* Remember the filename as the __file__ attribute */ + if (PyModule_AddObjectRef(mod, "__file__", info->path) < 0) { + PyErr_Clear(); /* Not important enough to report */ + } + + const char *name_buf = PyBytes_AS_STRING(info->name_encoded); + if (_PyImport_CheckSubinterpIncompatibleExtensionAllowed(name_buf) < 0) { + Py_CLEAR(mod); + goto finally; + } + + if (fix_up_extension(mod, res.def, info->name, info->path, NULL) < 0) { + Py_CLEAR(mod); + goto finally; + } + } + else { + mod = PyModule_FromDefAndSpec(res.def, spec); + } + + // XXX Shouldn't this happen in the error cases too. + if (fp) { + fclose(fp); + } + +finally: + return mod; +} + /*******************/ /* builtin modules */ @@ -3764,9 +3826,7 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) /*[clinic end generated code: output=83249b827a4fde77 input=c31b954f4cf4e09d]*/ { PyObject *mod = NULL; - FILE *fp; struct _Py_ext_module_loader_info info; - struct _Py_ext_module_loader_result res; if (_Py_ext_module_loader_info_init_from_spec(&info, spec) < 0) { return NULL; @@ -3786,56 +3846,10 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) } /* Is multi-phase init or this is the first time being loaded. */ - - /* We would move this (and the fclose() below) into - * _PyImport_GetModInitFunc(), but it isn't clear if the intervening - * code relies on fp still being open. */ - if (file != NULL) { - fp = _Py_fopen_obj(info.path, "r"); - if (fp == NULL) { - goto finally; - } - } - else { - fp = NULL; - } - - PyModInitFunction p0 = _PyImport_GetModInitFunc(&info, fp); - if (p0 == NULL) { + mod = create_dynamic(&info, file, spec); + if (mod == NULL) { goto finally; } - if (_PyImport_RunModInitFunc(p0, &info, &res) < 0) { - _Py_ext_module_loader_result_apply_error(&res); - return NULL; - } - - if (res.singlephase) { - mod = Py_NewRef(res.module); - - /* Remember the filename as the __file__ attribute */ - if (PyModule_AddObjectRef(mod, "__file__", info.path) < 0) { - PyErr_Clear(); /* Not important enough to report */ - } - - const char *name_buf = PyBytes_AS_STRING(info.name_encoded); - if (_PyImport_CheckSubinterpIncompatibleExtensionAllowed(name_buf) < 0) { - Py_CLEAR(mod); - goto finally; - } - - if (fix_up_extension(mod, res.def, info.name, info.path, NULL) < 0) { - Py_CLEAR(mod); - goto finally; - } - } - else { - mod = PyModule_FromDefAndSpec(res.def, spec); - } - - // XXX Shouldn't this happen in the error cases too. - if (fp) { - fclose(fp); - } finally: _Py_ext_module_loader_info_clear(&info); From 2705e6598e3f37db1b432cb888a4e4af36297ad3 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 16 Apr 2024 17:23:50 -0600 Subject: [PATCH 15/40] Drop _PyImport_FixupExtensionObject(). --- Include/internal/pycore_import.h | 5 +---- Include/moduleobject.h | 2 +- Python/import.c | 14 -------------- 3 files changed, 2 insertions(+), 19 deletions(-) diff --git a/Include/internal/pycore_import.h b/Include/internal/pycore_import.h index cb6c957c09fa7e..e35a1b22468559 100644 --- a/Include/internal/pycore_import.h +++ b/Include/internal/pycore_import.h @@ -27,9 +27,6 @@ extern int _PyImport_FixupBuiltin( const char *name, /* UTF-8 encoded string */ PyObject *modules ); -// We could probably drop this: -extern int _PyImport_FixupExtensionObject(PyObject*, PyObject *, - PyObject *, PyObject *); // Export for many shared extensions, like '_json' PyAPI_FUNC(PyObject*) _PyImport_GetModuleAttr(PyObject *, PyObject *); @@ -53,7 +50,7 @@ struct _import_runtime_state { Only legacy (single-phase init) extension modules are added and only if they support multiple initialization (m_size >- 0) or are imported in the main interpreter. - This is initialized lazily in _PyImport_FixupExtensionObject(). + This is initialized lazily in fix_up_extension() in import.c. Modules are added there and looked up in _imp.find_extension(). */ _Py_hashtable_t *hashtable; } extensions; diff --git a/Include/moduleobject.h b/Include/moduleobject.h index 42b87cc4e91012..83f8c2030dbb8f 100644 --- a/Include/moduleobject.h +++ b/Include/moduleobject.h @@ -53,7 +53,7 @@ typedef struct PyModuleDef_Base { /* A copy of the module's __dict__ after the first time it was loaded. This is only set/used for legacy modules that do not support multiple initializations. - It is set by _PyImport_FixupExtensionObject(). */ + It is set by fix_up_extension() in import.c. */ PyObject* m_copy; } PyModuleDef_Base; diff --git a/Python/import.c b/Python/import.c index bc4dd0faa887ae..45601d6d8727c0 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1252,20 +1252,6 @@ fix_up_extension(PyObject *mod, PyModuleDef *def, return 0; } -int -_PyImport_FixupExtensionObject(PyObject *mod, PyObject *name, - PyObject *filename, PyObject *modules) -{ - if (mod == NULL || !PyModule_Check(mod)) { - PyErr_BadInternalCall(); - return -1; - } - if (fix_up_extension(mod, NULL, name, filename, modules) < 0) { - return -1; - } - return 0; -} - static PyObject * import_find_extension(PyThreadState *tstate, PyObject *name, From 98d8b4b0b78339bb7911dc57d833bd95640ce7eb Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 17 Apr 2024 11:18:45 -0600 Subject: [PATCH 16/40] Move setting __file__ to fix_up_extension(). --- Python/import.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/Python/import.c b/Python/import.c index 45601d6d8727c0..9a2f659e5bef8d 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1223,6 +1223,17 @@ fix_up_extension(PyObject *mod, PyModuleDef *def, PyObject *name, PyObject *filename, PyObject *modules) { + if (filename != NULL) { + /* Remember the filename as the __file__ attribute */ + if (PyModule_AddObjectRef(mod, "__file__", filename) < 0) { + PyErr_Clear(); /* Not important enough to report */ + } + } + else { + /* It must be a builtin module. */ + filename = name; + } + PyInterpreterState *interp = _PyInterpreterState_GET(); if (def == NULL) { def = PyModule_GetDef(mod); @@ -1388,11 +1399,6 @@ create_dynamic(struct _Py_ext_module_loader_info *info, PyObject *file, if (res.singlephase) { mod = Py_NewRef(res.module); - /* Remember the filename as the __file__ attribute */ - if (PyModule_AddObjectRef(mod, "__file__", info->path) < 0) { - PyErr_Clear(); /* Not important enough to report */ - } - const char *name_buf = PyBytes_AS_STRING(info->name_encoded); if (_PyImport_CheckSubinterpIncompatibleExtensionAllowed(name_buf) < 0) { Py_CLEAR(mod); @@ -1432,7 +1438,7 @@ _PyImport_FixupBuiltin(PyObject *mod, const char *name, PyObject *modules) return -1; } assert(mod != NULL && PyModule_Check(mod)); - if (fix_up_extension(mod, NULL, nameobj, nameobj, modules) < 0) { + if (fix_up_extension(mod, NULL, nameobj, NULL, modules) < 0) { goto finally; } res = 0; @@ -1492,7 +1498,7 @@ create_builtin(PyThreadState *tstate, PyObject *name, PyObject *spec) } def->m_base.m_init = p->initfunc; - if (fix_up_extension(mod, def, name, name, modules) < 0) { + if (fix_up_extension(mod, def, name, NULL, modules) < 0) { return NULL; } return mod; From ddf113535b41d211cc8e8db43492303e6416333a Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 17 Apr 2024 11:31:31 -0600 Subject: [PATCH 17/40] Pass tstate through to fix_up_extension(). --- Include/internal/pycore_import.h | 1 + Python/import.c | 26 +++++++++++++++----------- Python/pylifecycle.c | 2 +- Python/sysmodule.c | 2 +- 4 files changed, 18 insertions(+), 13 deletions(-) diff --git a/Include/internal/pycore_import.h b/Include/internal/pycore_import.h index e35a1b22468559..2c53a6d9b79485 100644 --- a/Include/internal/pycore_import.h +++ b/Include/internal/pycore_import.h @@ -23,6 +23,7 @@ extern void _PyImport_AcquireLock(PyInterpreterState *interp); extern int _PyImport_ReleaseLock(PyInterpreterState *interp); extern int _PyImport_FixupBuiltin( + PyThreadState *tstate, PyObject *mod, const char *name, /* UTF-8 encoded string */ PyObject *modules diff --git a/Python/import.c b/Python/import.c index 9a2f659e5bef8d..641bc69d2bf9f2 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1219,7 +1219,7 @@ fix_up_extension_for_interpreter(PyInterpreterState *interp, static int -fix_up_extension(PyObject *mod, PyModuleDef *def, +fix_up_extension(PyThreadState *tstate, PyObject *mod, PyModuleDef *def, PyObject *name, PyObject *filename, PyObject *modules) { @@ -1234,7 +1234,6 @@ fix_up_extension(PyObject *mod, PyModuleDef *def, filename = name; } - PyInterpreterState *interp = _PyInterpreterState_GET(); if (def == NULL) { def = PyModule_GetDef(mod); if (def == NULL) { @@ -1244,7 +1243,7 @@ fix_up_extension(PyObject *mod, PyModuleDef *def, } // XXX Why special-case the main interpreter? - if (_Py_IsMainInterpreter(interp) || def->m_size == -1) { + if (_Py_IsMainInterpreter(tstate->interp) || def->m_size == -1) { #ifndef NDEBUG PyModuleDef *cached = _extensions_cache_get(filename, name); assert(cached == NULL || cached == def); @@ -1255,7 +1254,7 @@ fix_up_extension(PyObject *mod, PyModuleDef *def, } if (fix_up_extension_for_interpreter( - interp, mod, def, name, filename, modules) < 0) + tstate->interp, mod, def, name, filename, modules) < 0) { return -1; } @@ -1367,8 +1366,8 @@ clear_singlephase_extension(PyInterpreterState *interp, } static PyObject * -create_dynamic(struct _Py_ext_module_loader_info *info, PyObject *file, - PyObject *spec) +create_dynamic(PyThreadState *tstate, struct _Py_ext_module_loader_info *info, + PyObject *file, PyObject *spec) { FILE *fp; struct _Py_ext_module_loader_result res; @@ -1405,7 +1404,9 @@ create_dynamic(struct _Py_ext_module_loader_info *info, PyObject *file, goto finally; } - if (fix_up_extension(mod, res.def, info->name, info->path, NULL) < 0) { + if (fix_up_extension( + tstate, mod, res.def, info->name, info->path, NULL) < 0) + { Py_CLEAR(mod); goto finally; } @@ -1429,7 +1430,8 @@ create_dynamic(struct _Py_ext_module_loader_info *info, PyObject *file, /*******************/ int -_PyImport_FixupBuiltin(PyObject *mod, const char *name, PyObject *modules) +_PyImport_FixupBuiltin(PyThreadState *tstate, PyObject *mod, const char *name, + PyObject *modules) { int res = -1; PyObject *nameobj; @@ -1438,7 +1440,7 @@ _PyImport_FixupBuiltin(PyObject *mod, const char *name, PyObject *modules) return -1; } assert(mod != NULL && PyModule_Check(mod)); - if (fix_up_extension(mod, NULL, nameobj, NULL, modules) < 0) { + if (fix_up_extension(tstate, mod, NULL, nameobj, NULL, modules) < 0) { goto finally; } res = 0; @@ -1498,7 +1500,9 @@ create_builtin(PyThreadState *tstate, PyObject *name, PyObject *spec) } def->m_base.m_init = p->initfunc; - if (fix_up_extension(mod, def, name, NULL, modules) < 0) { + if (fix_up_extension( + tstate, mod, def, name, NULL, modules) < 0) + { return NULL; } return mod; @@ -3838,7 +3842,7 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) } /* Is multi-phase init or this is the first time being loaded. */ - mod = create_dynamic(&info, file, spec); + mod = create_dynamic(tstate, &info, file, spec); if (mod == NULL) { goto finally; } diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index efb25878312d85..eaca859dc24700 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -777,7 +777,7 @@ pycore_init_builtins(PyThreadState *tstate) } PyObject *modules = _PyImport_GetModules(interp); - if (_PyImport_FixupBuiltin(bimod, "builtins", modules) < 0) { + if (_PyImport_FixupBuiltin(tstate, bimod, "builtins", modules) < 0) { goto error; } diff --git a/Python/sysmodule.c b/Python/sysmodule.c index 7b4a643bccd1dd..b3c63057aa337f 100644 --- a/Python/sysmodule.c +++ b/Python/sysmodule.c @@ -3764,7 +3764,7 @@ _PySys_Create(PyThreadState *tstate, PyObject **sysmod_p) return status; } - if (_PyImport_FixupBuiltin(sysmod, "sys", modules) < 0) { + if (_PyImport_FixupBuiltin(tstate, sysmod, "sys", modules) < 0) { goto error; } From 694a76345e8cc4a6c69403ebf244269669386c9a Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 17 Apr 2024 12:34:16 -0600 Subject: [PATCH 18/40] Add a note about _PyImport_FixupBuiltin(). --- Include/internal/pycore_import.h | 1 + 1 file changed, 1 insertion(+) diff --git a/Include/internal/pycore_import.h b/Include/internal/pycore_import.h index 2c53a6d9b79485..b02769903a6f9b 100644 --- a/Include/internal/pycore_import.h +++ b/Include/internal/pycore_import.h @@ -22,6 +22,7 @@ extern int _PyImport_SetModuleString(const char *name, PyObject* module); extern void _PyImport_AcquireLock(PyInterpreterState *interp); extern int _PyImport_ReleaseLock(PyInterpreterState *interp); +// This is used exclusively for the sys and builtins modules: extern int _PyImport_FixupBuiltin( PyThreadState *tstate, PyObject *mod, From 72e0d73c0e3b18894d34a027cf75f5e1111fbc82 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 17 Apr 2024 12:43:19 -0600 Subject: [PATCH 19/40] Always expect a valid modules dict passed to fix_up_extension(). --- Python/import.c | 57 +++++++++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/Python/import.c b/Python/import.c index 641bc69d2bf9f2..cc51ff146fbfa4 100644 --- a/Python/import.c +++ b/Python/import.c @@ -201,44 +201,53 @@ _PyImport_ClearModules(PyInterpreterState *interp) } static inline PyObject * -get_modules_dict(PyInterpreterState *interp) +get_modules_dict(PyThreadState *tstate, bool fatal) { - if (MODULES(interp) == NULL) { - Py_FatalError("interpreter has no modules dictionary"); + /* Technically, it would make sense to incref the dict, + * since sys.modules could be swapped out and decref'ed to 0 + * before the caller is done using it. However, that is highly + * unlikely, especially since we can rely on a global lock + * (i.e. the GIL) for thread-safety. */ + PyObject *modules = MODULES(tstate->interp); + if (modules == NULL) { + if (fatal) { + Py_FatalError("interpreter has no modules dictionary"); + } + _PyErr_SetString(tstate, PyExc_RuntimeError, + "unable to get sys.modules"); + return NULL; } - return MODULES(interp); + return modules; } PyObject * PyImport_GetModuleDict(void) { - PyInterpreterState *interp = _PyInterpreterState_GET(); - return get_modules_dict(interp); + PyThreadState *tstate = _PyThreadState_GET(); + return get_modules_dict(tstate, true); } int _PyImport_SetModule(PyObject *name, PyObject *m) { - PyInterpreterState *interp = _PyInterpreterState_GET(); - PyObject *modules = MODULES(interp); + PyThreadState *tstate = _PyThreadState_GET(); + PyObject *modules = get_modules_dict(tstate, true); return PyObject_SetItem(modules, name, m); } int _PyImport_SetModuleString(const char *name, PyObject *m) { - PyInterpreterState *interp = _PyInterpreterState_GET(); - PyObject *modules = MODULES(interp); + PyThreadState *tstate = _PyThreadState_GET(); + PyObject *modules = get_modules_dict(tstate, true); return PyMapping_SetItemString(modules, name, m); } static PyObject * import_get_module(PyThreadState *tstate, PyObject *name) { - PyObject *modules = MODULES(tstate->interp); + PyObject *modules = get_modules_dict(tstate, false); if (modules == NULL) { - _PyErr_SetString(tstate, PyExc_RuntimeError, - "unable to get sys.modules"); return NULL; } @@ -303,10 +312,8 @@ PyImport_GetModule(PyObject *name) static PyObject * import_add_module(PyThreadState *tstate, PyObject *name) { - PyObject *modules = MODULES(tstate->interp); + PyObject *modules = get_modules_dict(tstate, false); if (modules == NULL) { - _PyErr_SetString(tstate, PyExc_RuntimeError, - "no import module dictionary"); return NULL; } @@ -403,7 +410,7 @@ remove_module(PyThreadState *tstate, PyObject *name) { PyObject *exc = _PyErr_GetRaisedException(tstate); - PyObject *modules = MODULES(tstate->interp); + PyObject *modules = get_modules_dict(tstate, true); if (PyDict_CheckExact(modules)) { // Error is reported to the caller (void)PyDict_Pop(modules, name, NULL); @@ -1168,17 +1175,16 @@ is_core_module(PyInterpreterState *interp, PyObject *name, PyObject *filename) } static int -fix_up_extension_for_interpreter(PyInterpreterState *interp, +fix_up_extension_for_interpreter(PyThreadState *tstate, PyObject *mod, PyModuleDef *def, PyObject *name, PyObject *filename, PyObject *modules) { + PyInterpreterState *interp = tstate->interp; assert(mod != NULL && PyModule_Check(mod)); assert(def == PyModule_GetDef(mod)); - if (modules == NULL) { - modules = get_modules_dict(interp); - } + assert(modules != NULL); if (PyObject_SetItem(modules, name, mod) < 0) { return -1; } @@ -1254,7 +1260,7 @@ fix_up_extension(PyThreadState *tstate, PyObject *mod, PyModuleDef *def, } if (fix_up_extension_for_interpreter( - tstate->interp, mod, def, name, filename, modules) < 0) + tstate, mod, def, name, filename, modules) < 0) { return -1; } @@ -1283,7 +1289,7 @@ import_find_extension(PyThreadState *tstate, PyObject *name, } PyObject *mod, *mdict; - PyObject *modules = MODULES(tstate->interp); + PyObject *modules = get_modules_dict(tstate, true); if (def->m_size == -1) { PyObject *m_copy = def->m_base.m_copy; @@ -1404,8 +1410,9 @@ create_dynamic(PyThreadState *tstate, struct _Py_ext_module_loader_info *info, goto finally; } + PyObject *modules = get_modules_dict(tstate, true); if (fix_up_extension( - tstate, mod, res.def, info->name, info->path, NULL) < 0) + tstate, mod, res.def, info->name, info->path, modules) < 0) { Py_CLEAR(mod); goto finally; @@ -1476,7 +1483,7 @@ create_builtin(PyThreadState *tstate, PyObject *name, PyObject *spec) return mod; } - PyObject *modules = MODULES(tstate->interp); + PyObject *modules = get_modules_dict(tstate, true); for (struct _inittab *p = INITTAB; p->name != NULL; p++) { if (_PyUnicode_EqualToASCIIString(name, p->name)) { if (p->initfunc == NULL) { From 3f14a67fa7bb8056fc630e914d0053570d52742e Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 17 Apr 2024 12:59:48 -0600 Subject: [PATCH 20/40] Factor out reload_singlephase_extension(). --- Python/import.c | 59 +++++++++++++++++++++++++++++++------------------ 1 file changed, 38 insertions(+), 21 deletions(-) diff --git a/Python/import.c b/Python/import.c index cc51ff146fbfa4..4547c69129eadc 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1270,25 +1270,10 @@ fix_up_extension(PyThreadState *tstate, PyObject *mod, PyModuleDef *def, static PyObject * -import_find_extension(PyThreadState *tstate, PyObject *name, - PyObject *filename) +reload_singlephase_extension(PyThreadState *tstate, PyModuleDef *def, + PyObject *name, PyObject *filename) { - /* Only single-phase init modules will be in the cache. */ - PyModuleDef *def = _extensions_cache_get(filename, name); - if (def == NULL) { - return NULL; - } - - /* It may have been successfully imported previously - in an interpreter that allows legacy modules - but is not allowed in the current interpreter. */ - const char *name_buf = PyUnicode_AsUTF8(name); - assert(name_buf != NULL); - if (_PyImport_CheckSubinterpIncompatibleExtensionAllowed(name_buf) < 0) { - return NULL; - } - - PyObject *mod, *mdict; + PyObject *mod; PyObject *modules = get_modules_dict(tstate, true); if (def->m_size == -1) { @@ -1299,6 +1284,7 @@ import_find_extension(PyThreadState *tstate, PyObject *name, for which we don't set m_copy. */ m_copy = get_core_module_dict(tstate->interp, name, filename); if (m_copy == NULL) { + assert(!PyErr_Occurred()); return NULL; } } @@ -1306,7 +1292,7 @@ import_find_extension(PyThreadState *tstate, PyObject *name, if (mod == NULL) { return NULL; } - mdict = PyModule_GetDict(mod); + PyObject *mdict = PyModule_GetDict(mod); if (mdict == NULL) { Py_DECREF(mod); return NULL; @@ -1317,22 +1303,53 @@ import_find_extension(PyThreadState *tstate, PyObject *name, } } else { - if (def->m_base.m_init == NULL) + if (def->m_base.m_init == NULL) { + assert(!PyErr_Occurred()); return NULL; + } mod = def->m_base.m_init(); - if (mod == NULL) + if (mod == NULL) { return NULL; + } if (PyObject_SetItem(modules, name, mod) == -1) { Py_DECREF(mod); return NULL; } } + if (_modules_by_index_set(tstate->interp, def, mod) < 0) { PyMapping_DelItem(modules, name); Py_DECREF(mod); return NULL; } + return mod; +} + +static PyObject * +import_find_extension(PyThreadState *tstate, PyObject *name, + PyObject *filename) +{ + /* Only single-phase init modules will be in the cache. */ + PyModuleDef *def = _extensions_cache_get(filename, name); + if (def == NULL) { + return NULL; + } + + /* It may have been successfully imported previously + in an interpreter that allows legacy modules + but is not allowed in the current interpreter. */ + const char *name_buf = PyUnicode_AsUTF8(name); + assert(name_buf != NULL); + if (_PyImport_CheckSubinterpIncompatibleExtensionAllowed(name_buf) < 0) { + return NULL; + } + + PyObject *mod = reload_singlephase_extension(tstate, def, name, filename); + if (mod == NULL) { + return NULL; + } + int verbose = _PyInterpreterState_GetConfig(tstate->interp)->verbose; if (verbose) { PySys_FormatStderr("import %U # previously loaded (%R)\n", From b5f58682bbfb8a09f51416b519c60cabdc8550a4 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 17 Apr 2024 13:43:37 -0600 Subject: [PATCH 21/40] res.singlephase -> res.kind --- Include/internal/pycore_importdl.h | 7 ++++++- Python/import.c | 10 ++++++---- Python/importdl.c | 7 ++++--- 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/Include/internal/pycore_importdl.h b/Include/internal/pycore_importdl.h index 4f06c27280698e..077d75b6745229 100644 --- a/Include/internal/pycore_importdl.h +++ b/Include/internal/pycore_importdl.h @@ -35,7 +35,12 @@ extern int _Py_ext_module_loader_info_init_from_spec( struct _Py_ext_module_loader_result { PyModuleDef *def; PyObject *module; - int singlephase; + enum { + _Py_ext_module_loader_result_UNKNOWN = 0, + _Py_ext_module_loader_result_SINGLEPHASE = 1, + _Py_ext_module_loader_result_MULTIPHASE = 2, + _Py_ext_module_loader_result_INVALID = 3, + } kind; char err[200]; }; extern void _Py_ext_module_loader_result_apply_error( diff --git a/Python/import.c b/Python/import.c index 4547c69129eadc..449bf770d70151 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1417,8 +1417,13 @@ create_dynamic(PyThreadState *tstate, struct _Py_ext_module_loader_info *info, _Py_ext_module_loader_result_apply_error(&res); goto finally; } + assert(!PyErr_Occurred()); - if (res.singlephase) { + if (res.kind == _Py_ext_module_loader_result_MULTIPHASE) { + mod = PyModule_FromDefAndSpec(res.def, spec); + } + else { + assert(res.kind == _Py_ext_module_loader_result_SINGLEPHASE); mod = Py_NewRef(res.module); const char *name_buf = PyBytes_AS_STRING(info->name_encoded); @@ -1435,9 +1440,6 @@ create_dynamic(PyThreadState *tstate, struct _Py_ext_module_loader_info *info, goto finally; } } - else { - mod = PyModule_FromDefAndSpec(res.def, spec); - } // XXX Shouldn't this happen in the error cases too. if (fp) { diff --git a/Python/importdl.c b/Python/importdl.c index c20a7c54b7fbe9..4710d8b9ad5674 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -210,7 +210,7 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, struct _Py_ext_module_loader_result *p_res) { struct _Py_ext_module_loader_result res = { - .singlephase=-1, + .kind=_Py_ext_module_loader_result_UNKNOWN, }; const char *name_buf = PyBytes_AS_STRING(info->name_encoded); @@ -251,6 +251,7 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, /* This can happen when a PyModuleDef is returned without calling * PyModuleDef_Init on it */ + res.kind = _Py_ext_module_loader_result_INVALID; SET_ERROR("init function of %s returned uninitialized object", name_buf); /* Likewise, decref'ing here makes sense. However, the original @@ -262,12 +263,12 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, if (PyObject_TypeCheck(m, &PyModuleDef_Type)) { /* multi-phase init */ - res.singlephase = 0; + res.kind = _Py_ext_module_loader_result_MULTIPHASE; res.def = (PyModuleDef *)m; } else { /* single-phase init (legacy) */ - res.singlephase = 1; + res.kind = _Py_ext_module_loader_result_SINGLEPHASE; res.module = m; /* Remember pointer to module init function. */ From a5f7535f9bb9e83bed392250d8f90000d4177936 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 17 Apr 2024 13:56:03 -0600 Subject: [PATCH 22/40] Factor out _extensions_cache_init(). --- Python/import.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/Python/import.c b/Python/import.c index 449bf770d70151..c52e48121c70a8 100644 --- a/Python/import.c +++ b/Python/import.c @@ -966,6 +966,25 @@ hashtable_destroy_str(void *ptr) #define HTSEP ':' +static int +_extensions_cache_init(void) +{ + _Py_hashtable_allocator_t alloc = {PyMem_RawMalloc, PyMem_RawFree}; + EXTENSIONS.hashtable = _Py_hashtable_new_full( + hashtable_hash_str, + hashtable_compare_str, + hashtable_destroy_str, // key + /* There's no need to decref the def since it's immortal. */ + NULL, // value + &alloc + ); + if (EXTENSIONS.hashtable == NULL) { + PyErr_NoMemory(); + return -1; + } + return 0; +} + static PyModuleDef * _extensions_cache_get(PyObject *filename, PyObject *name) { @@ -1003,17 +1022,7 @@ _extensions_cache_set(PyObject *filename, PyObject *name, PyModuleDef *def) extensions_lock_acquire(); if (EXTENSIONS.hashtable == NULL) { - _Py_hashtable_allocator_t alloc = {PyMem_RawMalloc, PyMem_RawFree}; - EXTENSIONS.hashtable = _Py_hashtable_new_full( - hashtable_hash_str, - hashtable_compare_str, - hashtable_destroy_str, // key - /* There's no need to decref the def since it's immortal. */ - NULL, // value - &alloc - ); - if (EXTENSIONS.hashtable == NULL) { - PyErr_NoMemory(); + if (_extensions_cache_init() < 0) { goto finally; } } From e8d674a856cda44a2c2e468358743812d1a54c51 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 17 Apr 2024 17:44:57 -0600 Subject: [PATCH 23/40] Factor out _extensions_cache_find_unlocked(). --- Python/import.c | 96 ++++++++++++++++++++++++++----------------------- 1 file changed, 52 insertions(+), 44 deletions(-) diff --git a/Python/import.c b/Python/import.c index c52e48121c70a8..f282c51de33cfd 100644 --- a/Python/import.c +++ b/Python/import.c @@ -985,40 +985,53 @@ _extensions_cache_init(void) return 0; } +static _Py_hashtable_entry_t * +_extensions_cache_find_unlocked(PyObject *filename, PyObject *name, + void **p_key) +{ + if (EXTENSIONS.hashtable == NULL) { + return NULL; + } + void *key = hashtable_key_from_2_strings(filename, name, HTSEP); + if (key == NULL) { + return NULL; + } + _Py_hashtable_entry_t *entry = + _Py_hashtable_get_entry(EXTENSIONS.hashtable, key); + if (p_key != NULL) { + *p_key = key; + } + else { + hashtable_destroy_str(key); + } + return entry; +} + static PyModuleDef * _extensions_cache_get(PyObject *filename, PyObject *name) { PyModuleDef *def = NULL; - void *key = NULL; extensions_lock_acquire(); - if (EXTENSIONS.hashtable == NULL) { - goto finally; - } - - key = hashtable_key_from_2_strings(filename, name, HTSEP); - if (key == NULL) { - goto finally; - } - _Py_hashtable_entry_t *entry = _Py_hashtable_get_entry( - EXTENSIONS.hashtable, key); + _Py_hashtable_entry_t *entry = + _extensions_cache_find_unlocked(filename, name, NULL); if (entry == NULL) { + /* It was never added. */ goto finally; } def = (PyModuleDef *)entry->value; finally: extensions_lock_release(); - if (key != NULL) { - PyMem_RawFree(key); - } return def; } static int -_extensions_cache_set(PyObject *filename, PyObject *name, PyModuleDef *def) +_extensions_cache_set(PyObject *filename, PyObject *name, PyModuleDef *def, + bool replace) { int res = -1; + assert(def != NULL); extensions_lock_acquire(); if (EXTENSIONS.hashtable == NULL) { @@ -1027,32 +1040,33 @@ _extensions_cache_set(PyObject *filename, PyObject *name, PyModuleDef *def) } } - void *key = hashtable_key_from_2_strings(filename, name, HTSEP); - if (key == NULL) { - goto finally; - } - int already_set = 0; - _Py_hashtable_entry_t *entry = _Py_hashtable_get_entry( - EXTENSIONS.hashtable, key); + void *key = NULL; + _Py_hashtable_entry_t *entry = + _extensions_cache_find_unlocked(filename, name, &key); if (entry == NULL) { + /* It was never added. */ if (_Py_hashtable_set(EXTENSIONS.hashtable, key, def) < 0) { - PyMem_RawFree(key); PyErr_NoMemory(); goto finally; } + /* The hashtable owns the key now. */ + key = NULL; + } + else if (entry->value == NULL) { + /* It was previously deleted. */ + entry->value = def; + } + /* We expect it to be static, so it must be the same pointer. */ + else if ((PyModuleDef *)entry->value == def) { + /* It was already added. */ + already_set = 1; } else { - if (entry->value == NULL) { - entry->value = def; - } - else { - /* We expect it to be static, so it must be the same pointer. */ - assert((PyModuleDef *)entry->value == def); - already_set = 1; - } - PyMem_RawFree(key); + assert(replace); + entry->value = def; } + if (!already_set) { /* We assume that all module defs are statically allocated and will never be freed. Otherwise, we would incref here. */ @@ -1062,13 +1076,15 @@ _extensions_cache_set(PyObject *filename, PyObject *name, PyModuleDef *def) finally: extensions_lock_release(); + if (key != NULL) { + hashtable_destroy_str(key); + } return res; } static void _extensions_cache_delete(PyObject *filename, PyObject *name) { - void *key = NULL; extensions_lock_acquire(); if (EXTENSIONS.hashtable == NULL) { @@ -1076,13 +1092,8 @@ _extensions_cache_delete(PyObject *filename, PyObject *name) goto finally; } - key = hashtable_key_from_2_strings(filename, name, HTSEP); - if (key == NULL) { - goto finally; - } - - _Py_hashtable_entry_t *entry = _Py_hashtable_get_entry( - EXTENSIONS.hashtable, key); + _Py_hashtable_entry_t *entry = + _extensions_cache_find_unlocked(filename, name, NULL); if (entry == NULL) { /* It was never added. */ goto finally; @@ -1100,9 +1111,6 @@ _extensions_cache_delete(PyObject *filename, PyObject *name) finally: extensions_lock_release(); - if (key != NULL) { - PyMem_RawFree(key); - } } static void @@ -1263,7 +1271,7 @@ fix_up_extension(PyThreadState *tstate, PyObject *mod, PyModuleDef *def, PyModuleDef *cached = _extensions_cache_get(filename, name); assert(cached == NULL || cached == def); #endif - if (_extensions_cache_set(filename, name, def) < 0) { + if (_extensions_cache_set(filename, name, def, false) < 0) { return -1; } } From 27b365df6a43cffbfa39b843c5e2bcab0ec608ab Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 17 Apr 2024 18:23:08 -0600 Subject: [PATCH 24/40] Use _PyImport_RunModInitFunc() in create_builtin(). --- Include/internal/pycore_importdl.h | 3 +++ Python/import.c | 30 ++++++++++++++----------- Python/importdl.c | 35 +++++++++++++++++++++++++++++- 3 files changed, 54 insertions(+), 14 deletions(-) diff --git a/Include/internal/pycore_importdl.h b/Include/internal/pycore_importdl.h index 077d75b6745229..0a918b4ba07ac8 100644 --- a/Include/internal/pycore_importdl.h +++ b/Include/internal/pycore_importdl.h @@ -28,6 +28,9 @@ struct _Py_ext_module_loader_info { }; extern void _Py_ext_module_loader_info_clear( struct _Py_ext_module_loader_info *info); +extern int _Py_ext_module_loader_info_init_for_builtin( + struct _Py_ext_module_loader_info *p_info, + PyObject *name); extern int _Py_ext_module_loader_info_init_from_spec( struct _Py_ext_module_loader_info *info, PyObject *spec); diff --git a/Python/import.c b/Python/import.c index f282c51de33cfd..5f565444046d67 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1526,25 +1526,29 @@ create_builtin(PyThreadState *tstate, PyObject *name, PyObject *spec) /* Cannot re-init internal module ("sys" or "builtins") */ return import_add_module(tstate, name); } - mod = (*p->initfunc)(); - if (mod == NULL) { + + struct _Py_ext_module_loader_info info; + if (_Py_ext_module_loader_info_init_for_builtin(&info, name) < 0) { return NULL; } - if (PyObject_TypeCheck(mod, &PyModuleDef_Type)) { - return PyModule_FromDefAndSpec((PyModuleDef*)mod, spec); + struct _Py_ext_module_loader_result res; + int rc = _PyImport_RunModInitFunc((*p->initfunc), &info, &res); + _Py_ext_module_loader_info_clear(&info); + if (rc < 0) { + _Py_ext_module_loader_result_apply_error(&res); + return NULL; + } + if (res.kind ==_Py_ext_module_loader_result_MULTIPHASE) { + assert(res.def != NULL); + assert(res.module == NULL); + return PyModule_FromDefAndSpec(res.def, spec); } else { - assert(PyModule_Check(mod)); - /* Remember pointer to module init function. */ - PyModuleDef *def = PyModule_GetDef(mod); - if (def == NULL) { - return NULL; - } - def->m_base.m_init = p->initfunc; - + assert(res.kind ==_Py_ext_module_loader_result_SINGLEPHASE); + mod = res.module; if (fix_up_extension( - tstate, mod, def, name, NULL, modules) < 0) + tstate, mod, res.def, name, NULL, modules) < 0) { return NULL; } diff --git a/Python/importdl.c b/Python/importdl.c index 4710d8b9ad5674..eb9003a3d73081 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -104,6 +104,33 @@ _Py_ext_module_loader_info_clear(struct _Py_ext_module_loader_info *info) Py_CLEAR(info->name_encoded); } +int +_Py_ext_module_loader_info_init_for_builtin( + struct _Py_ext_module_loader_info *info, + PyObject *name) +{ + assert(PyUnicode_Check(name)); + +#ifndef NDEBUG + Py_ssize_t name_len = PyUnicode_GetLength(name); +#endif + assert(name_len > 0); + assert(PyUnicode_FindChar(name, '.', 0, name_len, -1) == -1); + PyObject *name_encoded = PyUnicode_AsEncodedString(name, "ascii", NULL); + if (name_encoded == NULL) { + return -1; + } + + *info = (struct _Py_ext_module_loader_info){ + .name=Py_NewRef(name), + .name_encoded=name_encoded, + /* We won't need path. */ + .hook_prefix=ascii_only_prefix, + .newcontext=NULL, + }; + return 0; +} + int _Py_ext_module_loader_info_init_from_spec( struct _Py_ext_module_loader_info *p_info, @@ -271,10 +298,16 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, res.kind = _Py_ext_module_loader_result_SINGLEPHASE; res.module = m; + if (!PyModule_Check(m)) { + SET_ERROR("initialization of %s did not return an extension " + "module", name_buf); + goto error; + } + /* Remember pointer to module init function. */ res.def = PyModule_GetDef(m); if (res.def == NULL) { - SET_ERROR("initialization of %s did not return an extension " + SET_ERROR("initialization of %s did not return a valid extension " "module", name_buf); goto error; } From 512870232a57b4467b4423307241fbed1ee7866c Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 17 Apr 2024 18:25:23 -0600 Subject: [PATCH 25/40] Clean up _PyImport_FixupBuiltin(). --- Python/import.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/Python/import.c b/Python/import.c index 5f565444046d67..27569ea3dfa176 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1477,15 +1477,24 @@ _PyImport_FixupBuiltin(PyThreadState *tstate, PyObject *mod, const char *name, PyObject *modules) { int res = -1; + assert(mod != NULL && PyModule_Check(mod)); + PyObject *nameobj; nameobj = PyUnicode_InternFromString(name); if (nameobj == NULL) { return -1; } - assert(mod != NULL && PyModule_Check(mod)); - if (fix_up_extension(tstate, mod, NULL, nameobj, NULL, modules) < 0) { + + PyModuleDef *def = PyModule_GetDef(mod); + if (def == NULL) { + PyErr_BadInternalCall(); goto finally; } + + if (fix_up_extension(tstate, mod, def, nameobj, NULL, modules) < 0) { + goto finally; + } + res = 0; finally: From aaec36055a866b96c8f262f1ccb4c5dc0513f666 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 18 Apr 2024 15:09:46 -0600 Subject: [PATCH 26/40] Add some asserts to _PyImport_RunModInitFunc(). --- Python/importdl.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Python/importdl.c b/Python/importdl.c index eb9003a3d73081..0a992775eabfdf 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -307,6 +307,7 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, /* Remember pointer to module init function. */ res.def = PyModule_GetDef(m); if (res.def == NULL) { + PyErr_Clear(); SET_ERROR("initialization of %s did not return a valid extension " "module", name_buf); goto error; @@ -322,10 +323,12 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, } #undef SET_ERROR + assert(!PyErr_Occurred()); *p_res = res; return 0; error: + assert((PyErr_Occurred() == NULL) != (res.err[0] == '\0')); Py_CLEAR(res.module); res.def = NULL; *p_res = res; From f97671ef08b97f1e3e6bd0f9b4e50d3c53ffe578 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 18 Apr 2024 15:14:05 -0600 Subject: [PATCH 27/40] Drop the replace arg to _extensions_cache_set(). --- Python/import.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/Python/import.c b/Python/import.c index 27569ea3dfa176..4e41c6e31c26e3 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1027,8 +1027,7 @@ _extensions_cache_get(PyObject *filename, PyObject *name) } static int -_extensions_cache_set(PyObject *filename, PyObject *name, PyModuleDef *def, - bool replace) +_extensions_cache_set(PyObject *filename, PyObject *name, PyModuleDef *def) { int res = -1; assert(def != NULL); @@ -1057,15 +1056,12 @@ _extensions_cache_set(PyObject *filename, PyObject *name, PyModuleDef *def, /* It was previously deleted. */ entry->value = def; } - /* We expect it to be static, so it must be the same pointer. */ - else if ((PyModuleDef *)entry->value == def) { + else { + /* We expect it to be static, so it must be the same pointer. */ + assert((PyModuleDef *)entry->value == def); /* It was already added. */ already_set = 1; } - else { - assert(replace); - entry->value = def; - } if (!already_set) { /* We assume that all module defs are statically allocated @@ -1271,7 +1267,7 @@ fix_up_extension(PyThreadState *tstate, PyObject *mod, PyModuleDef *def, PyModuleDef *cached = _extensions_cache_get(filename, name); assert(cached == NULL || cached == def); #endif - if (_extensions_cache_set(filename, name, def, false) < 0) { + if (_extensions_cache_set(filename, name, def) < 0) { return -1; } } From 23669837d2c3006556247c4f2738b3ea83706bea Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 18 Apr 2024 16:24:25 -0600 Subject: [PATCH 28/40] Check the module returned by import_find_extension() to ensure single-phase init. --- Include/internal/pycore_importdl.h | 2 +- Python/import.c | 41 +++++++++++++++++++++++++----- 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/Include/internal/pycore_importdl.h b/Include/internal/pycore_importdl.h index 0a918b4ba07ac8..12b7d6ac1ae84a 100644 --- a/Include/internal/pycore_importdl.h +++ b/Include/internal/pycore_importdl.h @@ -38,7 +38,7 @@ extern int _Py_ext_module_loader_info_init_from_spec( struct _Py_ext_module_loader_result { PyModuleDef *def; PyObject *module; - enum { + enum _Py_ext_module_loader_result_kind { _Py_ext_module_loader_result_UNKNOWN = 0, _Py_ext_module_loader_result_SINGLEPHASE = 1, _Py_ext_module_loader_result_MULTIPHASE = 2, diff --git a/Python/import.c b/Python/import.c index 4e41c6e31c26e3..b1a275d4896808 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1187,6 +1187,22 @@ is_core_module(PyInterpreterState *interp, PyObject *name, PyObject *filename) return 0; } + +static enum _Py_ext_module_loader_result_kind +get_extension_kind(PyModuleDef *def) +{ + assert(def != NULL); + enum _Py_ext_module_loader_result_kind kind; + if (def->m_slots == NULL) { + kind = _Py_ext_module_loader_result_SINGLEPHASE; + } + else { + kind = _Py_ext_module_loader_result_MULTIPHASE; + } + return kind; +} + + static int fix_up_extension_for_interpreter(PyThreadState *tstate, PyObject *mod, PyModuleDef *def, @@ -1520,9 +1536,15 @@ static PyObject* create_builtin(PyThreadState *tstate, PyObject *name, PyObject *spec) { PyObject *mod = import_find_extension(tstate, name, name); - if (mod || _PyErr_Occurred(tstate)) { + if (mod != NULL) { + assert(!_PyErr_Occurred(tstate)); + assert(get_extension_kind(_PyModule_GetDef(mod)) + == _Py_ext_module_loader_result_SINGLEPHASE); return mod; } + else if (_PyErr_Occurred(tstate)) { + return NULL; + } PyObject *modules = get_modules_dict(tstate, true); for (struct _inittab *p = INITTAB; p->name != NULL; p++) { @@ -3873,19 +3895,24 @@ static PyObject * _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) /*[clinic end generated code: output=83249b827a4fde77 input=c31b954f4cf4e09d]*/ { - PyObject *mod = NULL; - struct _Py_ext_module_loader_info info; + PyThreadState *tstate = _PyThreadState_GET(); + struct _Py_ext_module_loader_info info; if (_Py_ext_module_loader_info_init_from_spec(&info, spec) < 0) { return NULL; } - PyThreadState *tstate = _PyThreadState_GET(); - mod = import_find_extension(tstate, info.name, info.path); - if (mod != NULL || _PyErr_Occurred(tstate)) { - assert(mod == NULL || !_PyErr_Occurred(tstate)); + PyObject *mod = import_find_extension(tstate, info.name, info.path); + if (mod != NULL) { + assert(!_PyErr_Occurred(tstate)); + assert(get_extension_kind(_PyModule_GetDef(mod)) + == _Py_ext_module_loader_result_SINGLEPHASE); + goto finally; + } + else if (_PyErr_Occurred(tstate)) { goto finally; } + /* Otherwise it must be multi-phase init or the first time it's loaded. */ if (PySys_Audit("import", "OOOOO", info.name, info.path, Py_None, Py_None, Py_None) < 0) From d5039eac961d4cc7d1e7c90c5d7211fc893ce12f Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 18 Apr 2024 18:13:33 -0600 Subject: [PATCH 29/40] Allow mod->md_def to be NULL. --- Python/import.c | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/Python/import.c b/Python/import.c index b1a275d4896808..e6e15fa0d69dde 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1191,9 +1191,13 @@ is_core_module(PyInterpreterState *interp, PyObject *name, PyObject *filename) static enum _Py_ext_module_loader_result_kind get_extension_kind(PyModuleDef *def) { - assert(def != NULL); enum _Py_ext_module_loader_result_kind kind; - if (def->m_slots == NULL) { + if (def == NULL) { + /* It must be a module created by reload_singlephase_extension() + * from m_copy. Ideally we'd do away with this case. */ + kind = _Py_ext_module_loader_result_SINGLEPHASE; + } + else if (def->m_slots == NULL) { kind = _Py_ext_module_loader_result_SINGLEPHASE; } else { @@ -1222,12 +1226,26 @@ fix_up_extension_for_interpreter(PyThreadState *tstate, goto error; } - // bpo-44050: Extensions and def->m_base.m_copy can be updated + // gh-88216: Extensions and def->m_base.m_copy can be updated // when the extension module doesn't support sub-interpreters. if (def->m_size == -1) { if (!is_core_module(interp, name, filename)) { assert(PyUnicode_CompareWithASCIIString(name, "sys") != 0); assert(PyUnicode_CompareWithASCIIString(name, "builtins") != 0); + /* XXX gh-88216: The copied dict is owned by the current + * interpreter. That's a problem if the interpreter has + * its own obmalloc state or if the module is successfully + * imported into such an interpreter. If the interpreter + * has its own GIL then there may be data races and + * PyImport_ClearModulesByIndex() can crash. Normally, + * a single-phase init module cannot be imported in an + * isolated interpreter, but there are ways around that. + * Hence, heere be dragons! Ideally we would instead do + * something like make a read-only, immortal copy of the + * dict using PyMem_RawMalloc() and store *that* in m_copy. + * Then we'd need to make sure to clear that when the + * runtime is finalized, rather than in + * PyImport_ClearModulesByIndex(). */ if (def->m_base.m_copy) { /* Somebody already imported the module, likely under a different name. @@ -1330,6 +1348,13 @@ reload_singlephase_extension(PyThreadState *tstate, PyModuleDef *def, Py_DECREF(mod); return NULL; } + /* We can't set mod->md_def if it's missing, + * because _PyImport_ClearModulesByIndex() might break + * due to violating interpreter isolation. See the note + * in fix_up_extension_for_interpreter(). Until that + * is solved, we leave md_def set to NULL. */ + assert(_PyModule_GetDef(mod) == NULL + || _PyModule_GetDef(mod) == def); } else { if (def->m_base.m_init == NULL) { From 67d217b270b21acc35374ad277c67bdc97ab8085 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 18 Apr 2024 18:15:02 -0600 Subject: [PATCH 30/40] Call _PyImport_RunModInitFunc() in reload_singlephase_extension(). --- Python/import.c | 68 +++++++++++++++++++++++++++++++------------------ 1 file changed, 43 insertions(+), 25 deletions(-) diff --git a/Python/import.c b/Python/import.c index e6e15fa0d69dde..d7181edf7ad795 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1318,10 +1318,11 @@ fix_up_extension(PyThreadState *tstate, PyObject *mod, PyModuleDef *def, static PyObject * reload_singlephase_extension(PyThreadState *tstate, PyModuleDef *def, - PyObject *name, PyObject *filename) + struct _Py_ext_module_loader_info *info) { PyObject *mod; PyObject *modules = get_modules_dict(tstate, true); + PyObject *filename = info->path == NULL ? info->name : info->path; if (def->m_size == -1) { PyObject *m_copy = def->m_base.m_copy; @@ -1329,13 +1330,13 @@ reload_singlephase_extension(PyThreadState *tstate, PyModuleDef *def, if (m_copy == NULL) { /* It might be a core module (e.g. sys & builtins), for which we don't set m_copy. */ - m_copy = get_core_module_dict(tstate->interp, name, filename); + m_copy = get_core_module_dict(tstate->interp, info->name, filename); if (m_copy == NULL) { assert(!PyErr_Occurred()); return NULL; } } - mod = import_add_module(tstate, name); + mod = import_add_module(tstate, info->name); if (mod == NULL) { return NULL; } @@ -1361,18 +1362,23 @@ reload_singlephase_extension(PyThreadState *tstate, PyModuleDef *def, assert(!PyErr_Occurred()); return NULL; } - mod = def->m_base.m_init(); - if (mod == NULL) { + struct _Py_ext_module_loader_result res; + if (_PyImport_RunModInitFunc(def->m_base.m_init, info, &res) < 0) { + _Py_ext_module_loader_result_apply_error(&res); return NULL; } - if (PyObject_SetItem(modules, name, mod) == -1) { + assert(!PyErr_Occurred()); + assert(res.kind == _Py_ext_module_loader_result_SINGLEPHASE); + mod = res.module; + // XXX __file__ doesn't get set! + if (PyObject_SetItem(modules, info->name, mod) == -1) { Py_DECREF(mod); return NULL; } } if (_modules_by_index_set(tstate->interp, def, mod) < 0) { - PyMapping_DelItem(modules, name); + PyMapping_DelItem(modules, info->name); Py_DECREF(mod); return NULL; } @@ -1381,11 +1387,13 @@ reload_singlephase_extension(PyThreadState *tstate, PyModuleDef *def, } static PyObject * -import_find_extension(PyThreadState *tstate, PyObject *name, - PyObject *filename) +import_find_extension(PyThreadState *tstate, + struct _Py_ext_module_loader_info *info) { + PyObject *filename = info->path == NULL ? info->name : info->path; + /* Only single-phase init modules will be in the cache. */ - PyModuleDef *def = _extensions_cache_get(filename, name); + PyModuleDef *def = _extensions_cache_get(filename, info->name); if (def == NULL) { return NULL; } @@ -1393,13 +1401,13 @@ import_find_extension(PyThreadState *tstate, PyObject *name, /* It may have been successfully imported previously in an interpreter that allows legacy modules but is not allowed in the current interpreter. */ - const char *name_buf = PyUnicode_AsUTF8(name); + const char *name_buf = PyUnicode_AsUTF8(info->name); assert(name_buf != NULL); if (_PyImport_CheckSubinterpIncompatibleExtensionAllowed(name_buf) < 0) { return NULL; } - PyObject *mod = reload_singlephase_extension(tstate, def, name, filename); + PyObject *mod = reload_singlephase_extension(tstate, def, info); if (mod == NULL) { return NULL; } @@ -1407,7 +1415,7 @@ import_find_extension(PyThreadState *tstate, PyObject *name, int verbose = _PyInterpreterState_GetConfig(tstate->interp)->verbose; if (verbose) { PySys_FormatStderr("import %U # previously loaded (%R)\n", - name, filename); + info->name, filename); } return mod; } @@ -1560,15 +1568,20 @@ is_builtin(PyObject *name) static PyObject* create_builtin(PyThreadState *tstate, PyObject *name, PyObject *spec) { - PyObject *mod = import_find_extension(tstate, name, name); + struct _Py_ext_module_loader_info info; + if (_Py_ext_module_loader_info_init_for_builtin(&info, name) < 0) { + return NULL; + } + + PyObject *mod = import_find_extension(tstate, &info); if (mod != NULL) { assert(!_PyErr_Occurred(tstate)); assert(get_extension_kind(_PyModule_GetDef(mod)) == _Py_ext_module_loader_result_SINGLEPHASE); - return mod; + goto finally; } else if (_PyErr_Occurred(tstate)) { - return NULL; + goto finally; } PyObject *modules = get_modules_dict(tstate, true); @@ -1576,12 +1589,13 @@ create_builtin(PyThreadState *tstate, PyObject *name, PyObject *spec) if (_PyUnicode_EqualToASCIIString(name, p->name)) { if (p->initfunc == NULL) { /* Cannot re-init internal module ("sys" or "builtins") */ - return import_add_module(tstate, name); + mod = import_add_module(tstate, name); + goto finally; } struct _Py_ext_module_loader_info info; if (_Py_ext_module_loader_info_init_for_builtin(&info, name) < 0) { - return NULL; + goto finally; } struct _Py_ext_module_loader_result res; @@ -1589,12 +1603,13 @@ create_builtin(PyThreadState *tstate, PyObject *name, PyObject *spec) _Py_ext_module_loader_info_clear(&info); if (rc < 0) { _Py_ext_module_loader_result_apply_error(&res); - return NULL; + goto finally; } if (res.kind ==_Py_ext_module_loader_result_MULTIPHASE) { assert(res.def != NULL); assert(res.module == NULL); - return PyModule_FromDefAndSpec(res.def, spec); + mod = PyModule_FromDefAndSpec(res.def, spec); + goto finally; } else { assert(res.kind ==_Py_ext_module_loader_result_SINGLEPHASE); @@ -1602,15 +1617,18 @@ create_builtin(PyThreadState *tstate, PyObject *name, PyObject *spec) if (fix_up_extension( tstate, mod, res.def, name, NULL, modules) < 0) { - return NULL; + Py_CLEAR(mod); } - return mod; + goto finally; } } } - // not found - Py_RETURN_NONE; + mod = Py_None; + +finally: + _Py_ext_module_loader_info_clear(&info); + return mod; } @@ -3927,7 +3945,7 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) return NULL; } - PyObject *mod = import_find_extension(tstate, info.name, info.path); + PyObject *mod = import_find_extension(tstate, &info); if (mod != NULL) { assert(!_PyErr_Occurred(tstate)); assert(get_extension_kind(_PyModule_GetDef(mod)) From 9e40287d0333d2b7e0c3ad2ab8177f50d29d233c Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 18 Apr 2024 18:15:35 -0600 Subject: [PATCH 31/40] res->err is an array, not just a pointer. --- Python/importdl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Python/importdl.c b/Python/importdl.c index 0a992775eabfdf..dde33f5d209768 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -183,7 +183,7 @@ void _Py_ext_module_loader_result_apply_error( struct _Py_ext_module_loader_result *res) { - if (res->err != NULL) { + if (res->err[0] != '\0') { if (PyErr_Occurred()) { _PyErr_FormatFromCause(PyExc_SystemError, res->err); } @@ -328,7 +328,7 @@ _PyImport_RunModInitFunc(PyModInitFunction p0, return 0; error: - assert((PyErr_Occurred() == NULL) != (res.err[0] == '\0')); + assert(PyErr_Occurred() || res.err[0] != '\0'); Py_CLEAR(res.module); res.def = NULL; *p_res = res; From b8c2a1d0adc282afc61915743dcf3a7746ab5586 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 18 Apr 2024 16:34:25 -0600 Subject: [PATCH 32/40] Factor out import_run_extension() (and drop create_dynamic()). --- Python/import.c | 138 +++++++++++++++++++++++------------------------- 1 file changed, 67 insertions(+), 71 deletions(-) diff --git a/Python/import.c b/Python/import.c index d7181edf7ad795..2f383cec5eb717 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1451,42 +1451,37 @@ clear_singlephase_extension(PyInterpreterState *interp, } static PyObject * -create_dynamic(PyThreadState *tstate, struct _Py_ext_module_loader_info *info, - PyObject *file, PyObject *spec) +import_run_extension(PyThreadState *tstate, PyModInitFunction p0, + struct _Py_ext_module_loader_info *info, + PyObject *spec, PyObject *modules) { - FILE *fp; - struct _Py_ext_module_loader_result res; PyObject *mod = NULL; + PyModuleDef *def = NULL; - /* We would move this (and the fclose() below) into - * _PyImport_GetModInitFunc(), but it isn't clear if the intervening - * code relies on fp still being open. */ - if (file != NULL) { - fp = _Py_fopen_obj(info->path, "r"); - if (fp == NULL) { - goto finally; - } - } - else { - fp = NULL; - } - - PyModInitFunction p0 = _PyImport_GetModInitFunc(info, fp); - if (p0 == NULL) { - goto finally; - } + struct _Py_ext_module_loader_result res; if (_PyImport_RunModInitFunc(p0, info, &res) < 0) { + /* We discard res.def. */ + assert(res.module == NULL); _Py_ext_module_loader_result_apply_error(&res); goto finally; } assert(!PyErr_Occurred()); - if (res.kind == _Py_ext_module_loader_result_MULTIPHASE) { - mod = PyModule_FromDefAndSpec(res.def, spec); + mod = res.module; + res.module = NULL; + def = res.def; + + if (res.kind ==_Py_ext_module_loader_result_MULTIPHASE) { + assert(def != NULL); + assert(mod == NULL); + mod = PyModule_FromDefAndSpec(def, spec); + if (mod == NULL) { + goto finally; + } } else { - assert(res.kind == _Py_ext_module_loader_result_SINGLEPHASE); - mod = Py_NewRef(res.module); + assert(res.kind ==_Py_ext_module_loader_result_SINGLEPHASE); + assert(PyModule_Check(mod)); const char *name_buf = PyBytes_AS_STRING(info->name_encoded); if (_PyImport_CheckSubinterpIncompatibleExtensionAllowed(name_buf) < 0) { @@ -1494,20 +1489,14 @@ create_dynamic(PyThreadState *tstate, struct _Py_ext_module_loader_info *info, goto finally; } - PyObject *modules = get_modules_dict(tstate, true); if (fix_up_extension( - tstate, mod, res.def, info->name, info->path, modules) < 0) + tstate, mod, def, info->name, info->path, modules) < 0) { Py_CLEAR(mod); goto finally; } } - // XXX Shouldn't this happen in the error cases too. - if (fp) { - fclose(fp); - } - finally: return mod; } @@ -1584,47 +1573,30 @@ create_builtin(PyThreadState *tstate, PyObject *name, PyObject *spec) goto finally; } - PyObject *modules = get_modules_dict(tstate, true); + /* Look up the module in the inittab. */ + struct _inittab *found = NULL; for (struct _inittab *p = INITTAB; p->name != NULL; p++) { if (_PyUnicode_EqualToASCIIString(name, p->name)) { - if (p->initfunc == NULL) { - /* Cannot re-init internal module ("sys" or "builtins") */ - mod = import_add_module(tstate, name); - goto finally; - } - - struct _Py_ext_module_loader_info info; - if (_Py_ext_module_loader_info_init_for_builtin(&info, name) < 0) { - goto finally; - } - - struct _Py_ext_module_loader_result res; - int rc = _PyImport_RunModInitFunc((*p->initfunc), &info, &res); - _Py_ext_module_loader_info_clear(&info); - if (rc < 0) { - _Py_ext_module_loader_result_apply_error(&res); - goto finally; - } - if (res.kind ==_Py_ext_module_loader_result_MULTIPHASE) { - assert(res.def != NULL); - assert(res.module == NULL); - mod = PyModule_FromDefAndSpec(res.def, spec); - goto finally; - } - else { - assert(res.kind ==_Py_ext_module_loader_result_SINGLEPHASE); - mod = res.module; - if (fix_up_extension( - tstate, mod, res.def, name, NULL, modules) < 0) - { - Py_CLEAR(mod); - } - goto finally; - } + found = p; + break; } } - // not found - mod = Py_None; + if (found == NULL) { + // not found + mod = Py_NewRef(Py_None); + goto finally; + } + + PyModInitFunction p0 = (*found->initfunc); + if (p0 == NULL) { + /* Cannot re-init internal module ("sys" or "builtins") */ + mod = import_add_module(tstate, name); + goto finally; + } + + /* Now load it. */ + mod = import_run_extension( + tstate, p0, &info, spec, get_modules_dict(tstate, true)); finally: _Py_ext_module_loader_info_clear(&info); @@ -3938,6 +3910,7 @@ static PyObject * _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) /*[clinic end generated code: output=83249b827a4fde77 input=c31b954f4cf4e09d]*/ { + FILE *fp; PyThreadState *tstate = _PyThreadState_GET(); struct _Py_ext_module_loader_info info; @@ -3963,12 +3936,35 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) goto finally; } - /* Is multi-phase init or this is the first time being loaded. */ - mod = create_dynamic(tstate, &info, file, spec); + /* We would move this (and the fclose() below) into + * _PyImport_GetModInitFunc(), but it isn't clear if the intervening + * code relies on fp still being open. */ + if (file != NULL) { + fp = _Py_fopen_obj(info.path, "r"); + if (fp == NULL) { + goto finally; + } + } + else { + fp = NULL; + } + + PyModInitFunction p0 = _PyImport_GetModInitFunc(&info, fp); + if (p0 == NULL) { + goto finally; + } + + mod = import_run_extension( + tstate, p0, &info, spec, get_modules_dict(tstate, true)); if (mod == NULL) { goto finally; } + // XXX Shouldn't this happen in the error cases too (i.e. in "finally")? + if (fp) { + fclose(fp); + } + finally: _Py_ext_module_loader_info_clear(&info); return mod; From 967c649f6ed1007eeb30a63d9a28d4a57167525c Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 18 Apr 2024 18:39:08 -0600 Subject: [PATCH 33/40] Factor out update_extension_cache() and drop fix_up_extension_for_interpreter(). --- Python/import.c | 66 ++++++++++++++++++++++--------------------------- 1 file changed, 29 insertions(+), 37 deletions(-) diff --git a/Python/import.c b/Python/import.c index 2f383cec5eb717..5ffb822a23ee0a 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1208,28 +1208,13 @@ get_extension_kind(PyModuleDef *def) static int -fix_up_extension_for_interpreter(PyThreadState *tstate, - PyObject *mod, PyModuleDef *def, - PyObject *name, PyObject *filename, - PyObject *modules) +update_extensions_cache(PyThreadState *tstate, PyModuleDef *def, PyObject *mod, + PyObject *filename, PyObject *name) { - PyInterpreterState *interp = tstate->interp; - assert(mod != NULL && PyModule_Check(mod)); - assert(def == PyModule_GetDef(mod)); - - assert(modules != NULL); - if (PyObject_SetItem(modules, name, mod) < 0) { - return -1; - } - - if (_modules_by_index_set(interp, def, mod) < 0) { - goto error; - } - // gh-88216: Extensions and def->m_base.m_copy can be updated // when the extension module doesn't support sub-interpreters. if (def->m_size == -1) { - if (!is_core_module(interp, name, filename)) { + if (!is_core_module(tstate->interp, name, filename)) { assert(PyUnicode_CompareWithASCIIString(name, "sys") != 0); assert(PyUnicode_CompareWithASCIIString(name, "builtins") != 0); /* XXX gh-88216: The copied dict is owned by the current @@ -1254,28 +1239,36 @@ fix_up_extension_for_interpreter(PyThreadState *tstate, } PyObject *dict = PyModule_GetDict(mod); if (dict == NULL) { - goto error; + return -1; } def->m_base.m_copy = PyDict_Copy(dict); if (def->m_base.m_copy == NULL) { - goto error; + return -1; } } } - return 0; + // XXX Why special-case the main interpreter? + if (_Py_IsMainInterpreter(tstate->interp) || def->m_size == -1) { +#ifndef NDEBUG + PyModuleDef *cached = _extensions_cache_get(filename, name); + assert(cached == NULL || cached == def); +#endif + if (_extensions_cache_set(filename, name, def) < 0) { + return -1; + } + } -error: - PyMapping_DelItem(modules, name); - return -1; + return 0; } - static int fix_up_extension(PyThreadState *tstate, PyObject *mod, PyModuleDef *def, PyObject *name, PyObject *filename, PyObject *modules) { + assert(mod != NULL && PyModule_Check(mod)); + if (filename != NULL) { /* Remember the filename as the __file__ attribute */ if (PyModule_AddObjectRef(mod, "__file__", filename) < 0) { @@ -1294,21 +1287,20 @@ fix_up_extension(PyThreadState *tstate, PyObject *mod, PyModuleDef *def, return -1; } } + else { + assert(def == PyModule_GetDef(mod)); + } - // XXX Why special-case the main interpreter? - if (_Py_IsMainInterpreter(tstate->interp) || def->m_size == -1) { -#ifndef NDEBUG - PyModuleDef *cached = _extensions_cache_get(filename, name); - assert(cached == NULL || cached == def); -#endif - if (_extensions_cache_set(filename, name, def) < 0) { - return -1; - } + if (update_extensions_cache(tstate, def, mod, filename, name) < 0) { + return -1; } - if (fix_up_extension_for_interpreter( - tstate, mod, def, name, filename, modules) < 0) - { + /* Make interpreter-specific fixes. */ + if (_modules_by_index_set(tstate->interp, def, mod) < 0) { + return -1; + } + assert(modules != NULL); + if (PyObject_SetItem(modules, name, mod) < 0) { return -1; } From aecf56caa0d039feb031d0ecff6c30a870aaf50f Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 18 Apr 2024 19:16:44 -0600 Subject: [PATCH 34/40] We already know the def is okay. --- Python/import.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/Python/import.c b/Python/import.c index 5ffb822a23ee0a..1a961ba9233934 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1268,6 +1268,7 @@ fix_up_extension(PyThreadState *tstate, PyObject *mod, PyModuleDef *def, PyObject *modules) { assert(mod != NULL && PyModule_Check(mod)); + assert(def == _PyModule_GetDef(mod)); if (filename != NULL) { /* Remember the filename as the __file__ attribute */ @@ -1280,17 +1281,6 @@ fix_up_extension(PyThreadState *tstate, PyObject *mod, PyModuleDef *def, filename = name; } - if (def == NULL) { - def = PyModule_GetDef(mod); - if (def == NULL) { - PyErr_BadInternalCall(); - return -1; - } - } - else { - assert(def == PyModule_GetDef(mod)); - } - if (update_extensions_cache(tstate, def, mod, filename, name) < 0) { return -1; } @@ -1462,9 +1452,9 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, mod = res.module; res.module = NULL; def = res.def; + assert(def != NULL); if (res.kind ==_Py_ext_module_loader_result_MULTIPHASE) { - assert(def != NULL); assert(mod == NULL); mod = PyModule_FromDefAndSpec(def, spec); if (mod == NULL) { From 0d03d2308cd9bfb5312d9d652ddce190f22e0e9a Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Fri, 19 Apr 2024 13:51:26 -0600 Subject: [PATCH 35/40] Revive fix_up_extension_for_interpreter(). --- Python/import.c | 58 +++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 49 insertions(+), 9 deletions(-) diff --git a/Python/import.c b/Python/import.c index 1a961ba9233934..647bfcb46a3d72 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1262,10 +1262,36 @@ update_extensions_cache(PyThreadState *tstate, PyModuleDef *def, PyObject *mod, return 0; } +static int +fix_up_extension_for_interpreter(PyThreadState *tstate, + PyObject *mod, PyModuleDef *def, + PyObject *name, PyObject *modules) +{ + assert(mod != NULL && PyModule_Check(mod)); + assert(def == _PyModule_GetDef(mod)); + assert(modules != NULL); + + if (_modules_by_index_set(tstate->interp, def, mod) < 0) { + return -1; + } + + if (PyObject_SetItem(modules, name, mod) < 0) { + return -1; + } + + return 0; +} + +struct interpreter_specific_info { + PyObject *modules; + PyObject *name; + PyModuleDef *def; +}; + static int fix_up_extension(PyThreadState *tstate, PyObject *mod, PyModuleDef *def, PyObject *name, PyObject *filename, - PyObject *modules) + struct interpreter_specific_info *fix_interp) { assert(mod != NULL && PyModule_Check(mod)); assert(def == _PyModule_GetDef(mod)); @@ -1286,12 +1312,13 @@ fix_up_extension(PyThreadState *tstate, PyObject *mod, PyModuleDef *def, } /* Make interpreter-specific fixes. */ - if (_modules_by_index_set(tstate->interp, def, mod) < 0) { - return -1; - } - assert(modules != NULL); - if (PyObject_SetItem(modules, name, mod) < 0) { - return -1; + if (fix_interp != NULL) { + if (fix_up_extension_for_interpreter( + tstate, mod, fix_interp->def, fix_interp->name, + fix_interp->modules) < 0) + { + return -1; + } } return 0; @@ -1471,8 +1498,14 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, goto finally; } + struct interpreter_specific_info interp_specific = { + .modules=modules, + .name=info->name, + .def=def, + }; if (fix_up_extension( - tstate, mod, def, info->name, info->path, modules) < 0) + tstate, mod, def, info->name, info->path, + &interp_specific) < 0) { Py_CLEAR(mod); goto finally; @@ -1507,7 +1540,14 @@ _PyImport_FixupBuiltin(PyThreadState *tstate, PyObject *mod, const char *name, goto finally; } - if (fix_up_extension(tstate, mod, def, nameobj, NULL, modules) < 0) { + struct interpreter_specific_info interp_specific = { + .modules=modules, + .name=nameobj, + .def=def, + }; + if (fix_up_extension( + tstate, mod, def, nameobj, NULL, &interp_specific) < 0) + { goto finally; } From cace1022c03527d6c36877e6700bbb2c9566d9e4 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Fri, 19 Apr 2024 14:20:48 -0600 Subject: [PATCH 36/40] Add a distinct _Py_ext_module_loader_info.path (and old path -> filename). --- Include/internal/pycore_importdl.h | 7 +++++-- Python/import.c | 30 ++++++++++++++---------------- Python/importdl.c | 22 +++++++++++++--------- 3 files changed, 32 insertions(+), 27 deletions(-) diff --git a/Include/internal/pycore_importdl.h b/Include/internal/pycore_importdl.h index 12b7d6ac1ae84a..972b1e27eabd0b 100644 --- a/Include/internal/pycore_importdl.h +++ b/Include/internal/pycore_importdl.h @@ -17,12 +17,15 @@ extern const char *_PyImport_DynLoadFiletab[]; typedef PyObject *(*PyModInitFunction)(void); struct _Py_ext_module_loader_info { - PyObject *path; + PyObject *filename; #ifndef MS_WINDOWS - PyObject *path_encoded; + PyObject *filename_encoded; #endif PyObject *name; PyObject *name_encoded; + /* path is always a borrowed ref of name or filename, + * depending on if it's builtin or not. */ + PyObject *path; const char *hook_prefix; const char *newcontext; }; diff --git a/Python/import.c b/Python/import.c index 647bfcb46a3d72..45543b16f79267 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1209,12 +1209,12 @@ get_extension_kind(PyModuleDef *def) static int update_extensions_cache(PyThreadState *tstate, PyModuleDef *def, PyObject *mod, - PyObject *filename, PyObject *name) + PyObject *path, PyObject *name) { // gh-88216: Extensions and def->m_base.m_copy can be updated // when the extension module doesn't support sub-interpreters. if (def->m_size == -1) { - if (!is_core_module(tstate->interp, name, filename)) { + if (!is_core_module(tstate->interp, name, path)) { assert(PyUnicode_CompareWithASCIIString(name, "sys") != 0); assert(PyUnicode_CompareWithASCIIString(name, "builtins") != 0); /* XXX gh-88216: The copied dict is owned by the current @@ -1251,10 +1251,10 @@ update_extensions_cache(PyThreadState *tstate, PyModuleDef *def, PyObject *mod, // XXX Why special-case the main interpreter? if (_Py_IsMainInterpreter(tstate->interp) || def->m_size == -1) { #ifndef NDEBUG - PyModuleDef *cached = _extensions_cache_get(filename, name); + PyModuleDef *cached = _extensions_cache_get(path, name); assert(cached == NULL || cached == def); #endif - if (_extensions_cache_set(filename, name, def) < 0) { + if (_extensions_cache_set(path, name, def) < 0) { return -1; } } @@ -1331,7 +1331,6 @@ reload_singlephase_extension(PyThreadState *tstate, PyModuleDef *def, { PyObject *mod; PyObject *modules = get_modules_dict(tstate, true); - PyObject *filename = info->path == NULL ? info->name : info->path; if (def->m_size == -1) { PyObject *m_copy = def->m_base.m_copy; @@ -1339,7 +1338,8 @@ reload_singlephase_extension(PyThreadState *tstate, PyModuleDef *def, if (m_copy == NULL) { /* It might be a core module (e.g. sys & builtins), for which we don't set m_copy. */ - m_copy = get_core_module_dict(tstate->interp, info->name, filename); + m_copy = get_core_module_dict( + tstate->interp, info->name, info->path); if (m_copy == NULL) { assert(!PyErr_Occurred()); return NULL; @@ -1399,10 +1399,8 @@ static PyObject * import_find_extension(PyThreadState *tstate, struct _Py_ext_module_loader_info *info) { - PyObject *filename = info->path == NULL ? info->name : info->path; - /* Only single-phase init modules will be in the cache. */ - PyModuleDef *def = _extensions_cache_get(filename, info->name); + PyModuleDef *def = _extensions_cache_get(info->path, info->name); if (def == NULL) { return NULL; } @@ -1424,16 +1422,16 @@ import_find_extension(PyThreadState *tstate, int verbose = _PyInterpreterState_GetConfig(tstate->interp)->verbose; if (verbose) { PySys_FormatStderr("import %U # previously loaded (%R)\n", - info->name, filename); + info->name, info->path); } return mod; } static int clear_singlephase_extension(PyInterpreterState *interp, - PyObject *name, PyObject *filename) + PyObject *name, PyObject *path) { - PyModuleDef *def = _extensions_cache_get(filename, name); + PyModuleDef *def = _extensions_cache_get(path, name); if (def == NULL) { if (PyErr_Occurred()) { return -1; @@ -1454,7 +1452,7 @@ clear_singlephase_extension(PyInterpreterState *interp, } /* Clear the cached module def. */ - _extensions_cache_delete(filename, name); + _extensions_cache_delete(path, name); return 0; } @@ -1504,7 +1502,7 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, .def=def, }; if (fix_up_extension( - tstate, mod, def, info->name, info->path, + tstate, mod, def, info->name, info->filename, &interp_specific) < 0) { Py_CLEAR(mod); @@ -3952,7 +3950,7 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) } /* Otherwise it must be multi-phase init or the first time it's loaded. */ - if (PySys_Audit("import", "OOOOO", info.name, info.path, + if (PySys_Audit("import", "OOOOO", info.name, info.filename, Py_None, Py_None, Py_None) < 0) { goto finally; @@ -3962,7 +3960,7 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) * _PyImport_GetModInitFunc(), but it isn't clear if the intervening * code relies on fp still being open. */ if (file != NULL) { - fp = _Py_fopen_obj(info.path, "r"); + fp = _Py_fopen_obj(info.filename, "r"); if (fp == NULL) { goto finally; } diff --git a/Python/importdl.c b/Python/importdl.c index dde33f5d209768..7bd761168f0afa 100644 --- a/Python/importdl.c +++ b/Python/importdl.c @@ -96,12 +96,13 @@ get_encoded_name(PyObject *name, const char **hook_prefix) { void _Py_ext_module_loader_info_clear(struct _Py_ext_module_loader_info *info) { - Py_CLEAR(info->path); + Py_CLEAR(info->filename); #ifndef MS_WINDOWS - Py_CLEAR(info->path_encoded); + Py_CLEAR(info->filename_encoded); #endif Py_CLEAR(info->name); Py_CLEAR(info->name_encoded); + info->path = NULL; } int @@ -124,6 +125,7 @@ _Py_ext_module_loader_info_init_for_builtin( *info = (struct _Py_ext_module_loader_info){ .name=Py_NewRef(name), .name_encoded=name_encoded, + .path=name, /* We won't need path. */ .hook_prefix=ascii_only_prefix, .newcontext=NULL, @@ -161,20 +163,22 @@ _Py_ext_module_loader_info_init_from_spec( return -1; } - info.path = PyObject_GetAttrString(spec, "origin"); - if (info.path == NULL) { + info.filename = PyObject_GetAttrString(spec, "origin"); + if (info.filename == NULL) { _Py_ext_module_loader_info_clear(&info); return -1; } #ifndef MS_WINDOWS - info.path_encoded = PyUnicode_EncodeFSDefault(info.path); - if (info.path_encoded == NULL) { + info.filename_encoded = PyUnicode_EncodeFSDefault(info.filename); + if (info.filename_encoded == NULL) { _Py_ext_module_loader_info_clear(&info); return -1; } #endif + info.path = info.filename; + *p_info = info; return 0; } @@ -204,10 +208,10 @@ _PyImport_GetModInitFunc(struct _Py_ext_module_loader_info *info, dl_funcptr exportfunc; #ifdef MS_WINDOWS exportfunc = _PyImport_FindSharedFuncptrWindows( - info->hook_prefix, name_buf, info->path, fp); + info->hook_prefix, name_buf, info->filename, fp); #else { - const char *path_buf = PyBytes_AS_STRING(info->path_encoded); + const char *path_buf = PyBytes_AS_STRING(info->filename_encoded); exportfunc = _PyImport_FindSharedFuncptr( info->hook_prefix, name_buf, path_buf, fp); } @@ -221,7 +225,7 @@ _PyImport_GetModInitFunc(struct _Py_ext_module_loader_info *info, "module export function (%s_%s)", info->hook_prefix, name_buf); if (msg != NULL) { - PyErr_SetImportError(msg, info->name, info->path); + PyErr_SetImportError(msg, info->name, info->filename); Py_DECREF(msg); } } From 3cf0db087b9986765807e5ca136ab5a6489a11a7 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Fri, 19 Apr 2024 14:21:16 -0600 Subject: [PATCH 37/40] Do not call update_extensions_cache() in fix_up_extension(). --- Python/import.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/Python/import.c b/Python/import.c index 45543b16f79267..f7cbc8b8c91a34 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1289,12 +1289,10 @@ struct interpreter_specific_info { }; static int -fix_up_extension(PyThreadState *tstate, PyObject *mod, PyModuleDef *def, - PyObject *name, PyObject *filename, +fix_up_extension(PyThreadState *tstate, PyObject *mod, PyObject *filename, struct interpreter_specific_info *fix_interp) { assert(mod != NULL && PyModule_Check(mod)); - assert(def == _PyModule_GetDef(mod)); if (filename != NULL) { /* Remember the filename as the __file__ attribute */ @@ -1302,14 +1300,6 @@ fix_up_extension(PyThreadState *tstate, PyObject *mod, PyModuleDef *def, PyErr_Clear(); /* Not important enough to report */ } } - else { - /* It must be a builtin module. */ - filename = name; - } - - if (update_extensions_cache(tstate, def, mod, filename, name) < 0) { - return -1; - } /* Make interpreter-specific fixes. */ if (fix_interp != NULL) { @@ -1502,8 +1492,14 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, .def=def, }; if (fix_up_extension( - tstate, mod, def, info->name, info->filename, - &interp_specific) < 0) + tstate, mod, info->filename, &interp_specific) < 0) + { + Py_CLEAR(mod); + goto finally; + } + + if (update_extensions_cache( + tstate, def, mod, info->path, info->name) < 0) { Py_CLEAR(mod); goto finally; @@ -1543,9 +1539,11 @@ _PyImport_FixupBuiltin(PyThreadState *tstate, PyObject *mod, const char *name, .name=nameobj, .def=def, }; - if (fix_up_extension( - tstate, mod, def, nameobj, NULL, &interp_specific) < 0) - { + if (fix_up_extension(tstate, mod, NULL, &interp_specific) < 0) { + goto finally; + } + + if (update_extensions_cache(tstate, def, mod, nameobj, nameobj) < 0) { goto finally; } From 9b3088a6b2312a153e1bdaa6b192e96697e9039b Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Fri, 19 Apr 2024 14:36:40 -0600 Subject: [PATCH 38/40] Let the modules arg to fix_up_extension_for_interpreter() be NULL. --- Python/import.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Python/import.c b/Python/import.c index f7cbc8b8c91a34..d55d0525844d93 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1269,14 +1269,15 @@ fix_up_extension_for_interpreter(PyThreadState *tstate, { assert(mod != NULL && PyModule_Check(mod)); assert(def == _PyModule_GetDef(mod)); - assert(modules != NULL); if (_modules_by_index_set(tstate->interp, def, mod) < 0) { return -1; } - if (PyObject_SetItem(modules, name, mod) < 0) { - return -1; + if (modules != NULL) { + if (PyObject_SetItem(modules, name, mod) < 0) { + return -1; + } } return 0; From 4ac1644a3558e52bc74e9d85fe279593cf078214 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Fri, 19 Apr 2024 15:18:59 -0600 Subject: [PATCH 39/40] Let the caller of update_extensions_cache() decide if m_copy should be populated. --- Python/import.c | 67 +++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 54 insertions(+), 13 deletions(-) diff --git a/Python/import.c b/Python/import.c index d55d0525844d93..e71de37f12355e 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1207,14 +1207,28 @@ get_extension_kind(PyModuleDef *def) } +struct cached_singlephase_info { + PyObject *m_dict; +}; + static int -update_extensions_cache(PyThreadState *tstate, PyModuleDef *def, PyObject *mod, - PyObject *path, PyObject *name) +update_extensions_cache(PyThreadState *tstate, PyModuleDef *def, + PyObject *path, PyObject *name, + struct cached_singlephase_info *singlephase) { - // gh-88216: Extensions and def->m_base.m_copy can be updated - // when the extension module doesn't support sub-interpreters. - if (def->m_size == -1) { - if (!is_core_module(tstate->interp, name, path)) { + /* Copy the module's __dict__, if applicable. */ + if (singlephase == NULL) { + assert(def->m_base.m_copy == NULL); + } + else { + assert(def->m_base.m_init != NULL + || is_core_module(tstate->interp, name, path)); + if (singlephase->m_dict != NULL) { + assert(PyDict_Check(singlephase->m_dict)); + // gh-88216: Extensions and def->m_base.m_copy can be updated + // when the extension module doesn't support sub-interpreters. + assert(def->m_size == -1); + assert(!is_core_module(tstate->interp, name, path)); assert(PyUnicode_CompareWithASCIIString(name, "sys") != 0); assert(PyUnicode_CompareWithASCIIString(name, "builtins") != 0); /* XXX gh-88216: The copied dict is owned by the current @@ -1237,12 +1251,10 @@ update_extensions_cache(PyThreadState *tstate, PyModuleDef *def, PyObject *mod, XXX this should really not happen. */ Py_CLEAR(def->m_base.m_copy); } - PyObject *dict = PyModule_GetDict(mod); - if (dict == NULL) { - return -1; - } - def->m_base.m_copy = PyDict_Copy(dict); + def->m_base.m_copy = PyDict_Copy(singlephase->m_dict); if (def->m_base.m_copy == NULL) { + // XXX Ignore this error? Doing so would effectively + // mark the module as not loadable. */ return -1; } } @@ -1455,6 +1467,7 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, { PyObject *mod = NULL; PyModuleDef *def = NULL; + bool iscore = is_core_module(tstate->interp, info->name, info->path); struct _Py_ext_module_loader_result res; if (_PyImport_RunModInitFunc(p0, info, &res) < 0) { @@ -1481,12 +1494,14 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, assert(res.kind ==_Py_ext_module_loader_result_SINGLEPHASE); assert(PyModule_Check(mod)); + /* Make sure this module is allowed in this interpreter. */ const char *name_buf = PyBytes_AS_STRING(info->name_encoded); if (_PyImport_CheckSubinterpIncompatibleExtensionAllowed(name_buf) < 0) { Py_CLEAR(mod); goto finally; } + /* Do any final fixes to the module. */ struct interpreter_specific_info interp_specific = { .modules=modules, .name=info->name, @@ -1499,8 +1514,16 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, goto finally; } + /* Add the module def to the global cache. */ + struct cached_singlephase_info singlephase = {0}; + // gh-88216: Extensions and def->m_base.m_copy can be updated + // when the extension module doesn't support sub-interpreters. + if (def->m_size == -1 && !iscore) { + singlephase.m_dict = PyModule_GetDict(mod); + assert(singlephase.m_dict != NULL); + } if (update_extensions_cache( - tstate, def, mod, info->path, info->name) < 0) + tstate, def, info->path, info->name, &singlephase) < 0) { Py_CLEAR(mod); goto finally; @@ -1535,6 +1558,17 @@ _PyImport_FixupBuiltin(PyThreadState *tstate, PyObject *mod, const char *name, goto finally; } + /* We only use _PyImport_FixupBuiltin() for the core builtin modules + * (sys and builtins). These modules are single-phase init with no + * module state, but we also don't populate def->m_base.m_copy + * for them. */ + assert(is_core_module(tstate->interp, nameobj, nameobj)); + assert(get_extension_kind(def) == + _Py_ext_module_loader_result_SINGLEPHASE); + assert(def->m_size == -1); + assert(def->m_base.m_copy == NULL); + + /* Do the normal fixes to the module. */ struct interpreter_specific_info interp_specific = { .modules=modules, .name=nameobj, @@ -1544,7 +1578,14 @@ _PyImport_FixupBuiltin(PyThreadState *tstate, PyObject *mod, const char *name, goto finally; } - if (update_extensions_cache(tstate, def, mod, nameobj, nameobj) < 0) { + /* Add the module def to the global cache. */ + struct cached_singlephase_info singlephase = { + /* We don't want def->m_base.m_copy populated. */ + .m_dict=NULL, + }; + if (update_extensions_cache( + tstate, def, nameobj, nameobj, &singlephase) < 0) + { goto finally; } From 393a177e8e3730852f99667b320ec497fea8c054 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Fri, 19 Apr 2024 17:00:00 -0600 Subject: [PATCH 40/40] Fix the update_extensions_cache() signature. --- Python/import.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Python/import.c b/Python/import.c index e71de37f12355e..74323c47bf192a 100644 --- a/Python/import.c +++ b/Python/import.c @@ -1212,8 +1212,8 @@ struct cached_singlephase_info { }; static int -update_extensions_cache(PyThreadState *tstate, PyModuleDef *def, - PyObject *path, PyObject *name, +update_extensions_cache(PyThreadState *tstate, + PyObject *path, PyObject *name, PyModuleDef *def, struct cached_singlephase_info *singlephase) { /* Copy the module's __dict__, if applicable. */ @@ -1523,7 +1523,7 @@ import_run_extension(PyThreadState *tstate, PyModInitFunction p0, assert(singlephase.m_dict != NULL); } if (update_extensions_cache( - tstate, def, info->path, info->name, &singlephase) < 0) + tstate, info->path, info->name, def, &singlephase) < 0) { Py_CLEAR(mod); goto finally; @@ -1584,7 +1584,7 @@ _PyImport_FixupBuiltin(PyThreadState *tstate, PyObject *mod, const char *name, .m_dict=NULL, }; if (update_extensions_cache( - tstate, def, nameobj, nameobj, &singlephase) < 0) + tstate, nameobj, nameobj, def, &singlephase) < 0) { goto finally; }