bpo-28411: Isolate PyInterpreterState.modules (#3575)

A bunch of code currently uses PyInterpreterState.modules directly instead of PyImport_GetModuleDict(). This complicates efforts to make changes relative to sys.modules. This patch switches to using PyImport_GetModuleDict() uniformly. Also, a number of related uses of sys.modules are updated for uniformity for the same reason.

Note that this code was already reviewed and merged as part of #1638. I reverted that and am now splitting it up into more focused parts.

Include/import.h

@@ -38,11 +38,17 @@ PyAPI_FUNC(PyObject *) PyImport_ExecCodeModuleObject(
     );
 #endif
 PyAPI_FUNC(PyObject *) PyImport_GetModuleDict(void);
+#ifndef Py_LIMITED_API
+PyAPI_FUNC(int) _PyImport_IsInitialized(PyInterpreterState *);
+#endif
 #if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x03030000
 PyAPI_FUNC(PyObject *) PyImport_AddModuleObject(
     PyObject *name
     );
 #endif
+#ifndef Py_LIMITED_API
+PyAPI_FUNC(PyObject *) _PyImport_AddModuleObject(PyObject *, PyObject *);
+#endif
 PyAPI_FUNC(PyObject *) PyImport_AddModule(
     const char *name            /* UTF-8 encoded string */
     );
@@ -92,14 +98,19 @@ PyAPI_FUNC(int) _PyImport_ReleaseLock(void);
 PyAPI_FUNC(void) _PyImport_ReInitLock(void);
 
 PyAPI_FUNC(PyObject *) _PyImport_FindBuiltin(
-    const char *name            /* UTF-8 encoded string */
+    const char *name,            /* UTF-8 encoded string */
+    PyObject *modules
     );
 PyAPI_FUNC(PyObject *) _PyImport_FindExtensionObject(PyObject *, PyObject *);
+PyAPI_FUNC(PyObject *) _PyImport_FindExtensionObjectEx(PyObject *, PyObject *,
+                                                       PyObject *);
 PyAPI_FUNC(int) _PyImport_FixupBuiltin(
     PyObject *mod,
-    const char *name            /* UTF-8 encoded string */
+    const char *name,            /* UTF-8 encoded string */
+    PyObject *modules
     );
-PyAPI_FUNC(int) _PyImport_FixupExtensionObject(PyObject*, PyObject *, PyObject *);
+PyAPI_FUNC(int) _PyImport_FixupExtensionObject(PyObject*, PyObject *,
+                                               PyObject *, PyObject *);
 
 struct _inittab {
     const char *name;           /* ASCII encoded string */

Include/modsupport.h

@@ -191,6 +191,10 @@ PyAPI_FUNC(int) PyModule_ExecDef(PyObject *module, PyModuleDef *def);
 
 PyAPI_FUNC(PyObject *) PyModule_Create2(struct PyModuleDef*,
                                      int apiver);
+#ifndef Py_LIMITED_API
+PyAPI_FUNC(PyObject *) _PyModule_CreateInitialized(struct PyModuleDef*,
+                                                   int apiver);
+#endif
 
 #ifdef Py_LIMITED_API
 #define PyModule_Create(module) \

Misc/NEWS.d/next/Core and Builtins/2017-09-11-09-24-21.bpo-28411.12SpAm.rst

@@ -0,0 +1,3 @@
+Change direct usage of PyInterpreterState.modules to PyImport_GetModuleDict().
+Also introduce more uniformity in other code that deals with sys.modules.
+This helps reduce complications when working on sys.modules.

Objects/moduleobject.c

@@ -161,12 +161,18 @@ _add_methods_to_object(PyObject *module, PyObject *name, PyMethodDef *functions)
 
 PyObject *
 PyModule_Create2(struct PyModuleDef* module, int module_api_version)
+{
+    if (!_PyImport_IsInitialized(PyThreadState_GET()->interp))
+        Py_FatalError("Python import machinery not initialized");
+    return _PyModule_CreateInitialized(module, module_api_version);
+}
+
+PyObject *
+_PyModule_CreateInitialized(struct PyModuleDef* module, int module_api_version)
 {
     const char* name;
     PyModuleObject *m;
-    PyInterpreterState *interp = PyThreadState_Get()->interp;
+
-    if (interp->modules == NULL)
-        Py_FatalError("Python import machinery not initialized");
     if (!PyModuleDef_Init(module))
         return NULL;
     name = module->m_name;

Objects/typeobject.c

@@ -3902,7 +3902,6 @@ import_copyreg(void)
 {
     PyObject *copyreg_str;
     PyObject *copyreg_module;
-    PyInterpreterState *interp = PyThreadState_GET()->interp;
     _Py_IDENTIFIER(copyreg);
 
     copyreg_str = _PyUnicode_FromId(&PyId_copyreg);
@@ -3914,7 +3913,8 @@ import_copyreg(void)
        by storing a reference to the cached module in a static variable, but
        this broke when multiple embedded interpreters were in use (see issue
        #17408 and #19088). */
-    copyreg_module = PyDict_GetItemWithError(interp->modules, copyreg_str);
+    PyObject *modules = PyImport_GetModuleDict();
+    copyreg_module = PyDict_GetItemWithError(modules, copyreg_str);
     if (copyreg_module != NULL) {
         Py_INCREF(copyreg_module);
         return copyreg_module;

Python/bltinmodule.c

@@ -2685,7 +2685,7 @@ _PyBuiltin_Init(void)
         PyType_Ready(&PyZip_Type) < 0)
         return NULL;
 
-    mod = PyModule_Create(&builtinsmodule);
+    mod = _PyModule_CreateInitialized(&builtinsmodule, PYTHON_API_VERSION);
     if (mod == NULL)
         return NULL;
     dict = PyModule_GetDict(mod);

Python/import.c

@@ -296,6 +296,17 @@ PyImport_GetModuleDict(void)
     return interp->modules;
 }
 
+/* In some corner cases it is important to be sure that the import
+   machinery has been initialized (or not cleaned up yet).  For
+   example, see issue #4236 and PyModule_Create2(). */
+
+int
+_PyImport_IsInitialized(PyInterpreterState *interp)
+{
+    if (interp->modules == NULL)
+        return 0;
+    return 1;
+}
 
 /* List of names to clear in sys */
 static const char * const sys_deletes[] = {
@@ -323,7 +334,7 @@ PyImport_Cleanup(void)
     Py_ssize_t pos;
     PyObject *key, *value, *dict;
     PyInterpreterState *interp = PyThreadState_GET()->interp;
-    PyObject *modules = interp->modules;
+    PyObject *modules = PyImport_GetModuleDict();
     PyObject *weaklist = NULL;
     const char * const *p;
 
@@ -511,9 +522,9 @@ PyImport_GetMagicTag(void)
 
 int
 _PyImport_FixupExtensionObject(PyObject *mod, PyObject *name,
-                               PyObject *filename)
+                                 PyObject *filename, PyObject *modules)
 {
-    PyObject *modules, *dict, *key;
+    PyObject *dict, *key;
     struct PyModuleDef *def;
     int res;
     if (extensions == NULL) {
@@ -530,7 +541,6 @@ _PyImport_FixupExtensionObject(PyObject *mod, PyObject *name,
         PyErr_BadInternalCall();
         return -1;
     }
-    modules = PyImport_GetModuleDict();
     if (PyDict_SetItem(modules, name, mod) < 0)
         return -1;
     if (_PyState_AddModule(mod, def) < 0) {
@@ -562,20 +572,28 @@ _PyImport_FixupExtensionObject(PyObject *mod, PyObject *name,
 }
 
 int
-_PyImport_FixupBuiltin(PyObject *mod, const char *name)
+_PyImport_FixupBuiltin(PyObject *mod, const char *name, PyObject *modules)
 {
     int res;
     PyObject *nameobj;
     nameobj = PyUnicode_InternFromString(name);
     if (nameobj == NULL)
         return -1;
-    res = _PyImport_FixupExtensionObject(mod, nameobj, nameobj);
+    res = _PyImport_FixupExtensionObject(mod, nameobj, nameobj, modules);
     Py_DECREF(nameobj);
     return res;
 }
 
 PyObject *
 _PyImport_FindExtensionObject(PyObject *name, PyObject *filename)
+{
+    PyObject *modules = PyImport_GetModuleDict();
+    return _PyImport_FindExtensionObjectEx(name, filename, modules);
+}
+
+PyObject *
+_PyImport_FindExtensionObjectEx(PyObject *name, PyObject *filename,
+                                PyObject *modules)
 {
     PyObject *mod, *mdict, *key;
     PyModuleDef* def;
@@ -592,7 +610,7 @@ _PyImport_FindExtensionObject(PyObject *name, PyObject *filename)
         /* Module does not support repeated initialization */
         if (def->m_base.m_copy == NULL)
             return NULL;
-        mod = PyImport_AddModuleObject(name);
+        mod = _PyImport_AddModuleObject(name, modules);
         if (mod == NULL)
             return NULL;
         mdict = PyModule_GetDict(mod);
@@ -607,14 +625,14 @@ _PyImport_FindExtensionObject(PyObject *name, PyObject *filename)
         mod = def->m_base.m_init();
         if (mod == NULL)
             return NULL;
-        if (PyDict_SetItem(PyImport_GetModuleDict(), name, mod) == -1) {
+        if (PyDict_SetItem(modules, name, mod) == -1) {
             Py_DECREF(mod);
             return NULL;
         }
         Py_DECREF(mod);
     }
     if (_PyState_AddModule(mod, def) < 0) {
-        PyDict_DelItem(PyImport_GetModuleDict(), name);
+        PyDict_DelItem(modules, name);
         Py_DECREF(mod);
         return NULL;
     }
@@ -626,13 +644,13 @@ _PyImport_FindExtensionObject(PyObject *name, PyObject *filename)
 }
 
 PyObject *
-_PyImport_FindBuiltin(const char *name)
+_PyImport_FindBuiltin(const char *name, PyObject *modules)
 {
     PyObject *res, *nameobj;
     nameobj = PyUnicode_InternFromString(name);
     if (nameobj == NULL)
         return NULL;
-    res = _PyImport_FindExtensionObject(nameobj, nameobj);
+    res = _PyImport_FindExtensionObjectEx(nameobj, nameobj, modules);
     Py_DECREF(nameobj);
     return res;
 }
@@ -647,6 +665,12 @@ PyObject *
 PyImport_AddModuleObject(PyObject *name)
 {
     PyObject *modules = PyImport_GetModuleDict();
+    return _PyImport_AddModuleObject(name, modules);
+}
+
+PyObject *
+_PyImport_AddModuleObject(PyObject *name, PyObject *modules)
+{
     PyObject *m;
 
     if ((m = PyDict_GetItemWithError(modules, name)) != NULL &&
@@ -1042,6 +1066,7 @@ _imp_create_builtin(PyObject *module, PyObject *spec)
         return NULL;
     }
 
+    PyObject *modules = NULL;
     for (p = PyImport_Inittab; p->name != NULL; p++) {
         PyModuleDef *def;
         if (_PyUnicode_EqualToASCIIString(name, p->name)) {
@@ -1067,7 +1092,11 @@ _imp_create_builtin(PyObject *module, PyObject *spec)
                     return NULL;
                 }
                 def->m_base.m_init = p->initfunc;
-                if (_PyImport_FixupExtensionObject(mod, name, name) < 0) {
+                if (modules == NULL) {
+                    modules = PyImport_GetModuleDict();
+                }
+                if (_PyImport_FixupExtensionObject(mod, name, name,
+                                                   modules) < 0) {
                     Py_DECREF(name);
                     return NULL;
                 }
@@ -1511,7 +1540,8 @@ PyImport_ImportModuleLevelObject(PyObject *name, PyObject *globals,
         Py_INCREF(abs_name);
     }
 
-    mod = PyDict_GetItem(interp->modules, abs_name);
+    PyObject *modules = PyImport_GetModuleDict();
+    mod = PyDict_GetItem(modules, abs_name);
     if (mod != NULL && mod != Py_None) {
         _Py_IDENTIFIER(__spec__);
         _Py_IDENTIFIER(_initializing);
@@ -1598,7 +1628,8 @@ PyImport_ImportModuleLevelObject(PyObject *name, PyObject *globals,
                     goto error;
                 }
 
-                final_mod = PyDict_GetItem(interp->modules, to_return);
+                PyObject *modules = PyImport_GetModuleDict();
+                final_mod = PyDict_GetItem(modules, to_return);
                 Py_DECREF(to_return);
                 if (final_mod == NULL) {
                     PyErr_Format(PyExc_KeyError,

Python/importdl.c

@@ -215,7 +215,8 @@ _PyImport_LoadDynamicModuleWithSpec(PyObject *spec, FILE *fp)
     else
         Py_INCREF(path);
 
-    if (_PyImport_FixupExtensionObject(m, name_unicode, path) < 0)
+    PyObject *modules = PyImport_GetModuleDict();
+    if (_PyImport_FixupExtensionObject(m, name_unicode, path, modules) < 0)
         goto error;
 
     Py_DECREF(name_unicode);

Python/pylifecycle.c

@@ -675,9 +675,20 @@ void _Py_InitializeCore(const _PyCoreConfig *config)
     if (!_PyFloat_Init())
         Py_FatalError("Py_InitializeCore: can't init float");
 
-    interp->modules = PyDict_New();
+    PyObject *modules = PyDict_New();
-    if (interp->modules == NULL)
+    if (modules == NULL)
         Py_FatalError("Py_InitializeCore: can't make modules dictionary");
+    interp->modules = modules;
+
+    sysmod = _PySys_BeginInit();
+    if (sysmod == NULL)
+        Py_FatalError("Py_InitializeCore: can't initialize sys");
+    interp->sysdict = PyModule_GetDict(sysmod);
+    if (interp->sysdict == NULL)
+        Py_FatalError("Py_InitializeCore: can't initialize sys dict");
+    Py_INCREF(interp->sysdict);
+    PyDict_SetItemString(interp->sysdict, "modules", modules);
+    _PyImport_FixupBuiltin(sysmod, "sys", modules);
 
     /* Init Unicode implementation; relies on the codec registry */
     if (_PyUnicode_Init() < 0)
@@ -689,7 +700,7 @@ void _Py_InitializeCore(const _PyCoreConfig *config)
     bimod = _PyBuiltin_Init();
     if (bimod == NULL)
         Py_FatalError("Py_InitializeCore: can't initialize builtins modules");
-    _PyImport_FixupBuiltin(bimod, "builtins");
+    _PyImport_FixupBuiltin(bimod, "builtins", modules);
     interp->builtins = PyModule_GetDict(bimod);
     if (interp->builtins == NULL)
         Py_FatalError("Py_InitializeCore: can't initialize builtins dict");
@@ -698,17 +709,6 @@ void _Py_InitializeCore(const _PyCoreConfig *config)
     /* initialize builtin exceptions */
     _PyExc_Init(bimod);
 
-    sysmod = _PySys_BeginInit();
-    if (sysmod == NULL)
-        Py_FatalError("Py_InitializeCore: can't initialize sys");
-    interp->sysdict = PyModule_GetDict(sysmod);
-    if (interp->sysdict == NULL)
-        Py_FatalError("Py_InitializeCore: can't initialize sys dict");
-    Py_INCREF(interp->sysdict);
-    _PyImport_FixupBuiltin(sysmod, "sys");
-    PyDict_SetItemString(interp->sysdict, "modules",
-                         interp->modules);
-
     /* Set up a preliminary stderr printer until we have enough
        infrastructure for the io module in place. */
     pstderr = PyFile_NewStdPrinter(fileno(stderr));
@@ -1211,9 +1211,23 @@ Py_NewInterpreter(void)
 
     /* XXX The following is lax in error checking */
 
-    interp->modules = PyDict_New();
+    PyObject *modules = PyDict_New();
+    if (modules == NULL)
+        Py_FatalError("Py_NewInterpreter: can't make modules dictionary");
+    interp->modules = modules;
 
-    bimod = _PyImport_FindBuiltin("builtins");
+    sysmod = _PyImport_FindBuiltin("sys", modules);
+    if (sysmod != NULL) {
+        interp->sysdict = PyModule_GetDict(sysmod);
+        if (interp->sysdict == NULL)
+            goto handle_error;
+        Py_INCREF(interp->sysdict);
+        PyDict_SetItemString(interp->sysdict, "modules", modules);
+        PySys_SetPath(Py_GetPath());
+        _PySys_EndInit(interp->sysdict);
+    }
+
+    bimod = _PyImport_FindBuiltin("builtins", modules);
     if (bimod != NULL) {
         interp->builtins = PyModule_GetDict(bimod);
         if (interp->builtins == NULL)
@@ -1224,18 +1238,9 @@ Py_NewInterpreter(void)
     /* initialize builtin exceptions */
     _PyExc_Init(bimod);
 
-    sysmod = _PyImport_FindBuiltin("sys");
     if (bimod != NULL && sysmod != NULL) {
         PyObject *pstderr;
 
-        interp->sysdict = PyModule_GetDict(sysmod);
-        if (interp->sysdict == NULL)
-            goto handle_error;
-        Py_INCREF(interp->sysdict);
-        _PySys_EndInit(interp->sysdict);
-        PySys_SetPath(Py_GetPath());
-        PyDict_SetItemString(interp->sysdict, "modules",
-                             interp->modules);
         /* Set up a preliminary stderr printer until we have enough
            infrastructure for the io module in place. */
         pstderr = PyFile_NewStdPrinter(fileno(stderr));
@@ -1911,9 +1916,8 @@ wait_for_thread_shutdown(void)
 {
     _Py_IDENTIFIER(_shutdown);
     PyObject *result;
-    PyThreadState *tstate = PyThreadState_GET();
+    PyObject *modules = PyImport_GetModuleDict();
-    PyObject *threading = PyMapping_GetItemString(tstate->interp->modules,
+    PyObject *threading = PyMapping_GetItemString(modules, "threading");
-                                                  "threading");
     if (threading == NULL) {
         /* threading not imported */
         PyErr_Clear();

Python/sysmodule.c

@@ -162,8 +162,9 @@ static PyObject *
 sys_displayhook(PyObject *self, PyObject *o)
 {
     PyObject *outf;
-    PyInterpreterState *interp = PyThreadState_GET()->interp;
+    PyObject *modules = PyImport_GetModuleDict();
-    PyObject *modules = interp->modules;
+    if (modules == NULL)
+        return NULL;
     PyObject *builtins;
     static PyObject *newline = NULL;
     int err;
@@ -1949,7 +1950,7 @@ _PySys_BeginInit(void)
     PyObject *m, *sysdict, *version_info;
     int res;
 
-    m = PyModule_Create(&sysmodule);
+    m = _PyModule_CreateInitialized(&sysmodule, PYTHON_API_VERSION);
     if (m == NULL)
         return NULL;
     sysdict = PyModule_GetDict(m);