From 25ba81950aad7b41b29ae07c0a19dc77b2f75cab Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sat, 10 Oct 2020 12:24:59 +0300 Subject: [PATCH 1/4] bpo-41994: Fix refcount issues in Python/import.c --- Include/cpython/import.h | 2 - Include/internal/pycore_import.h | 5 -- .../2020-10-10-14-16-03.bpo-41994.Xop8sV.rst | 1 + Python/import.c | 86 ++++++++++--------- 4 files changed, 48 insertions(+), 46 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2020-10-10-14-16-03.bpo-41994.Xop8sV.rst diff --git a/Include/cpython/import.h b/Include/cpython/import.h index 3b20a74c855dbf..bad68f0e0980d5 100644 --- a/Include/cpython/import.h +++ b/Include/cpython/import.h @@ -13,8 +13,6 @@ PyAPI_FUNC(int) _PyImport_SetModuleString(const char *name, PyObject* module); PyAPI_FUNC(void) _PyImport_AcquireLock(void); PyAPI_FUNC(int) _PyImport_ReleaseLock(void); -PyAPI_FUNC(PyObject *) _PyImport_FindExtensionObject(PyObject *, PyObject *); - PyAPI_FUNC(int) _PyImport_FixupBuiltin( PyObject *mod, const char *name, /* UTF-8 encoded string */ diff --git a/Include/internal/pycore_import.h b/Include/internal/pycore_import.h index 35a67abebac6f7..ad930b43b502d8 100644 --- a/Include/internal/pycore_import.h +++ b/Include/internal/pycore_import.h @@ -5,11 +5,6 @@ extern "C" { #endif -PyAPI_FUNC(PyObject *) _PyImport_FindBuiltin( - PyThreadState *tstate, - const char *name /* UTF-8 encoded string */ - ); - #ifdef HAVE_FORK extern PyStatus _PyImport_ReInitLock(void); #endif diff --git a/Misc/NEWS.d/next/Core and Builtins/2020-10-10-14-16-03.bpo-41994.Xop8sV.rst b/Misc/NEWS.d/next/Core and Builtins/2020-10-10-14-16-03.bpo-41994.Xop8sV.rst new file mode 100644 index 00000000000000..36d5011ee71a61 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2020-10-10-14-16-03.bpo-41994.Xop8sV.rst @@ -0,0 +1 @@ +Fixed possible leak in ``import`` when ``sys.modules`` is not a ``dict``. diff --git a/Python/import.c b/Python/import.c index 505688400ef3e3..b30128c664e771 100644 --- a/Python/import.c +++ b/Python/import.c @@ -672,7 +672,7 @@ PyImport_GetMagicTag(void) modules. A copy of the module's dictionary is stored by calling _PyImport_FixupExtensionObject() immediately after the module initialization function succeeds. A copy can be retrieved from there by calling - _PyImport_FindExtensionObject(). + import_find_extension(). Modules which do support multiple initialization set their m_size field to a non-negative number (indicating the size of the @@ -785,10 +785,14 @@ import_find_extension(PyThreadState *tstate, PyObject *name, if (mod == NULL) return NULL; mdict = PyModule_GetDict(mod); - if (mdict == NULL) + if (mdict == NULL) { + Py_DECREF(mod); return NULL; - if (PyDict_Update(mdict, def->m_base.m_copy)) + } + if (PyDict_Update(mdict, def->m_base.m_copy)) { + Py_DECREF(mod); return NULL; + } } else { if (def->m_base.m_init == NULL) @@ -800,10 +804,10 @@ import_find_extension(PyThreadState *tstate, PyObject *name, Py_DECREF(mod); return NULL; } - Py_DECREF(mod); } if (_PyState_AddModule(tstate, mod, def) < 0) { PyMapping_DelItem(modules, name); + Py_DECREF(mod); return NULL; } @@ -815,31 +819,10 @@ import_find_extension(PyThreadState *tstate, PyObject *name, return mod; } -PyObject * -_PyImport_FindExtensionObject(PyObject *name, PyObject *filename) -{ - PyThreadState *tstate = _PyThreadState_GET(); - return import_find_extension(tstate, name, filename); -} - - -PyObject * -_PyImport_FindBuiltin(PyThreadState *tstate, const char *name) -{ - PyObject *res, *nameobj; - nameobj = PyUnicode_InternFromString(name); - if (nameobj == NULL) - return NULL; - res = import_find_extension(tstate, nameobj, nameobj); - Py_DECREF(nameobj); - return res; -} /* Get the module object corresponding to a module name. First check the modules dictionary if there's one there, - if not, create a new one and insert it in the modules dictionary. - Because the former action is most common, THIS DOES NOT RETURN A - 'NEW' REFERENCE! */ + if not, create a new one and insert it in the modules dictionary. */ static PyObject * import_add_module(PyThreadState *tstate, PyObject *name) @@ -854,6 +837,7 @@ import_add_module(PyThreadState *tstate, PyObject *name) PyObject *m; if (PyDict_CheckExact(modules)) { m = PyDict_GetItemWithError(modules, name); + Py_XINCREF(m); } else { m = PyObject_GetItem(modules, name); @@ -869,6 +853,7 @@ import_add_module(PyThreadState *tstate, PyObject *name) if (m != NULL && PyModule_Check(m)) { return m; } + Py_XDECREF(m); m = PyModule_NewObject(name); if (m == NULL) return NULL; @@ -876,7 +861,6 @@ import_add_module(PyThreadState *tstate, PyObject *name) Py_DECREF(m); return NULL; } - Py_DECREF(m); /* Yes, it still exists, in modules! */ return m; } @@ -885,7 +869,25 @@ PyObject * PyImport_AddModuleObject(PyObject *name) { PyThreadState *tstate = _PyThreadState_GET(); - return import_add_module(tstate, name); + PyObject *mod = import_add_module(tstate, name); + if (mod) { + if (Py_REFCNT(mod) == 1) { + /* This check does not preven an undefined behavior in the + * following code, because the module can have references + * to itself. */ + PyErr_SetString(PyExc_RuntimeError, "Unexpected zero reference count"); + Py_DECREF(mod); + return NULL; + } + Py_DECREF(mod); + if (Py_REFCNT(mod) == 0) { + /* Strictly speaking, this is an undefined behafior, and + * the above check can crash. */ + PyErr_SetString(PyExc_RuntimeError, "Unexpected zero reference count"); + return NULL; + } + } + return mod; /* borrowed reference */ } @@ -1007,7 +1009,7 @@ static PyObject * module_dict_for_exec(PyThreadState *tstate, PyObject *name) { _Py_IDENTIFIER(__builtins__); - PyObject *m, *d = NULL; + PyObject *m, *d; m = import_add_module(tstate, name); if (m == NULL) @@ -1021,11 +1023,14 @@ module_dict_for_exec(PyThreadState *tstate, PyObject *name) PyEval_GetBuiltins()) != 0) { remove_module(tstate, name); + Py_DECREF(m); return NULL; } } - return d; /* Return a borrowed reference. */ + Py_INCREF(d); + Py_DECREF(m); + return d; } static PyObject * @@ -1069,8 +1074,10 @@ PyImport_ExecCodeModuleObject(PyObject *name, PyObject *co, PyObject *pathname, } external = PyObject_GetAttrString(tstate->interp->importlib, "_bootstrap_external"); - if (external == NULL) + if (external == NULL) { + Py_DECREF(d); return NULL; + } res = _PyObject_CallMethodIdObjArgs(external, &PyId__fix_up_module, d, name, pathname, cpathname, NULL); @@ -1079,6 +1086,7 @@ PyImport_ExecCodeModuleObject(PyObject *name, PyObject *co, PyObject *pathname, Py_DECREF(res); res = exec_code_in_module(tstate, name, d, co); } + Py_DECREF(d); return res; } @@ -1263,10 +1271,9 @@ _imp_create_builtin(PyObject *module, PyObject *spec) return NULL; } - mod = _PyImport_FindExtensionObject(name, name); + mod = import_find_extension(tstate, name, name); if (mod || _PyErr_Occurred(tstate)) { Py_DECREF(name); - Py_XINCREF(mod); return mod; } @@ -1429,10 +1436,12 @@ PyImport_ImportFrozenModuleObject(PyObject *name) d = PyModule_GetDict(m); l = PyList_New(0); if (l == NULL) { + Py_DECREF(m); goto err_return; } err = PyDict_SetItemString(d, "__path__", l); Py_DECREF(l); + Py_DECREF(m); if (err != 0) goto err_return; } @@ -1442,10 +1451,12 @@ PyImport_ImportFrozenModuleObject(PyObject *name) } m = exec_code_in_module(tstate, name, d, co); if (m == NULL) { + Py_DECREF(d); goto err_return; } Py_DECREF(co); Py_DECREF(m); + Py_DECREF(d); return 1; err_return: @@ -2135,7 +2146,6 @@ _imp_init_frozen_impl(PyObject *module, PyObject *name) { PyThreadState *tstate = _PyThreadState_GET(); int ret; - PyObject *m; ret = PyImport_ImportFrozenModuleObject(name); if (ret < 0) @@ -2143,9 +2153,7 @@ _imp_init_frozen_impl(PyObject *module, PyObject *name) if (ret == 0) { Py_RETURN_NONE; } - m = import_add_module(tstate, name); - Py_XINCREF(m); - return m; + return import_add_module(tstate, name); } /*[clinic input] @@ -2269,11 +2277,11 @@ _imp_create_dynamic_impl(PyObject *module, PyObject *spec, PyObject *file) return NULL; } - mod = _PyImport_FindExtensionObject(name, path); + PyThreadState *tstate = _PyThreadState_GET(); + mod = import_find_extension(tstate, name, path); if (mod != NULL || PyErr_Occurred()) { Py_DECREF(name); Py_DECREF(path); - Py_XINCREF(mod); return mod; } From 197b72067fec02357a7d59e3fecc998e9a18e06e Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Wed, 14 Oct 2020 12:20:41 +0300 Subject: [PATCH 2/4] Apply suggestions from code review Co-authored-by: Pablo Galindo --- Python/import.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Python/import.c b/Python/import.c index b30128c664e771..4213ace5c850d1 100644 --- a/Python/import.c +++ b/Python/import.c @@ -872,7 +872,7 @@ PyImport_AddModuleObject(PyObject *name) PyObject *mod = import_add_module(tstate, name); if (mod) { if (Py_REFCNT(mod) == 1) { - /* This check does not preven an undefined behavior in the + /* This check does not prevent an undefined behavior in the * following code, because the module can have references * to itself. */ PyErr_SetString(PyExc_RuntimeError, "Unexpected zero reference count"); @@ -881,7 +881,7 @@ PyImport_AddModuleObject(PyObject *name) } Py_DECREF(mod); if (Py_REFCNT(mod) == 0) { - /* Strictly speaking, this is an undefined behafior, and + /* Strictly speaking, this is undefined behafior, and * the above check can crash. */ PyErr_SetString(PyExc_RuntimeError, "Unexpected zero reference count"); return NULL; From 8f74153cd9c8fbfadfd759b1db4cb00a0c19ada6 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Wed, 14 Oct 2020 14:22:21 +0300 Subject: [PATCH 3/4] Use weak reference, --- Python/import.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/Python/import.c b/Python/import.c index 39bd6e86b183ff..ab06f0425669ab 100644 --- a/Python/import.c +++ b/Python/import.c @@ -871,21 +871,13 @@ PyImport_AddModuleObject(PyObject *name) PyThreadState *tstate = _PyThreadState_GET(); PyObject *mod = import_add_module(tstate, name); if (mod) { - if (Py_REFCNT(mod) == 1) { - /* This check does not prevent an undefined behavior in the - * following code, because the module can have references - * to itself. */ - PyErr_SetString(PyExc_RuntimeError, "Unexpected zero reference count"); - Py_DECREF(mod); - return NULL; - } + PyObject *ref = PyWeakref_NewRef(mod, NULL); Py_DECREF(mod); - if (Py_REFCNT(mod) == 0) { - /* Strictly speaking, this is undefined behafior, and - * the above check can crash. */ - PyErr_SetString(PyExc_RuntimeError, "Unexpected zero reference count"); + if (ref == NULL) { return NULL; } + mod = PyWeakref_GetObject(ref); + Py_DECREF(ref); } return mod; /* borrowed reference */ } From 03e405f5d6329ca87712ae95d15ff6d31a192467 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Tue, 29 Dec 2020 20:17:05 +0200 Subject: [PATCH 4/4] Return new reference from get_path_importer(). --- Python/import.c | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/Python/import.c b/Python/import.c index af87c5ad9c3344..75ac21df82da94 100644 --- a/Python/import.c +++ b/Python/import.c @@ -912,8 +912,7 @@ is_builtin(PyObject *name) that can handle the path item. Return None if no hook could; this tells our caller that the path based finder could not find a finder for this path item. Cache the result in - path_importer_cache. - Returns a borrowed reference. */ + path_importer_cache. */ static PyObject * get_path_importer(PyThreadState *tstate, PyObject *path_importer_cache, @@ -931,8 +930,10 @@ get_path_importer(PyThreadState *tstate, PyObject *path_importer_cache, return NULL; /* Shouldn't happen */ importer = PyDict_GetItemWithError(path_importer_cache, p); - if (importer != NULL || _PyErr_Occurred(tstate)) + if (importer != NULL || _PyErr_Occurred(tstate)) { + Py_XINCREF(importer); return importer; + } /* set path_importer_cache[p] to None to avoid recursion */ if (PyDict_SetItem(path_importer_cache, p, Py_None) != 0) @@ -952,13 +953,11 @@ get_path_importer(PyThreadState *tstate, PyObject *path_importer_cache, _PyErr_Clear(tstate); } if (importer == NULL) { - return Py_None; + Py_RETURN_NONE; } - if (importer != NULL) { - int err = PyDict_SetItem(path_importer_cache, p, importer); + if (PyDict_SetItem(path_importer_cache, p, importer) < 0) { Py_DECREF(importer); - if (err != 0) - return NULL; + return NULL; } return importer; } @@ -967,16 +966,12 @@ PyObject * PyImport_GetImporter(PyObject *path) { PyThreadState *tstate = _PyThreadState_GET(); - PyObject *importer=NULL, *path_importer_cache=NULL, *path_hooks=NULL; - - path_importer_cache = PySys_GetObject("path_importer_cache"); - path_hooks = PySys_GetObject("path_hooks"); - if (path_importer_cache != NULL && path_hooks != NULL) { - importer = get_path_importer(tstate, path_importer_cache, - path_hooks, path); + PyObject *path_importer_cache = PySys_GetObject("path_importer_cache"); + PyObject *path_hooks = PySys_GetObject("path_hooks"); + if (path_importer_cache == NULL || path_hooks == NULL) { + return NULL; } - Py_XINCREF(importer); /* get_path_importer returns a borrowed reference */ - return importer; + return get_path_importer(tstate, path_importer_cache, path_hooks, path); } static PyObject* @@ -1178,13 +1173,12 @@ PyImport_ImportFrozenModuleObject(PyObject *name) goto err_return; } m = exec_code_in_module(tstate, name, d, co); + Py_DECREF(d); if (m == NULL) { - Py_DECREF(d); goto err_return; } Py_DECREF(co); Py_DECREF(m); - Py_DECREF(d); return 1; err_return: