Skip to content

gh-105922: PyImport_AddModule() uses Py_DECREF() #105998

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions Lib/test/test_import/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2644,6 +2644,29 @@ def test_pyimport_addmodule_create(self):
mod = _testcapi.check_pyimport_addmodule(name)
self.assertIs(mod, sys.modules[name])

def test_pyimport_addmodule_borrowed_ref(self):
# gh-105922: Test PyImport_AddModule() with a custom sys.modules which
# doesn't keep a strong reference to the newly created module
import _testcapi
module_name = 'dontexist'

self.assertNotIn(module_name, sys.modules)
self.addCleanup(unload, module_name)

class CustomModules(dict):
def __setitem__(self, key, value):
if key == module_name:
value = "REPLACE_VALUE"
super().__setitem__(key, value)

custom_modules = CustomModules()
with swap_attr(sys, 'modules', custom_modules):
_testcapi.pyimport_addmodule(module_name)

if module_name in sys.modules:
print("sys.modules CANNOT be overriden: "
"{module_name} is in sys.modules")


if __name__ == '__main__':
# Test needs to be a package, so we can do relative imports.
Expand Down
21 changes: 21 additions & 0 deletions Modules/_testcapimodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -3372,6 +3372,26 @@ check_pyimport_addmodule(PyObject *self, PyObject *args)
}


static PyObject *
test_pyimport_addmodule(PyObject *self, PyObject *args)
{
const char *name;
if (!PyArg_ParseTuple(args, "s", &name)) {
return NULL;
}
PyObject *mod = PyImport_AddModule(name);
if (mod == NULL) {
if (!PyErr_Occurred()) {
PyErr_SetString(PyExc_AssertionError,
"PyImport_AddModule returns NULL "
"without setting an exception");
}
return NULL;
}
return Py_NewRef(mod);
}


static PyObject *
test_weakref_capi(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args))
{
Expand Down Expand Up @@ -3610,6 +3630,7 @@ static PyMethodDef TestMethods[] = {
{"function_set_kw_defaults", function_set_kw_defaults, METH_VARARGS, NULL},
{"test_atexit", test_atexit, METH_NOARGS},
{"check_pyimport_addmodule", check_pyimport_addmodule, METH_VARARGS},
{"pyimport_addmodule", test_pyimport_addmodule, METH_VARARGS},
{"test_weakref_capi", test_weakref_capi, METH_NOARGS},
{NULL, NULL} /* sentinel */
};
Expand Down
16 changes: 10 additions & 6 deletions Python/import.c
Original file line number Diff line number Diff line change
Expand Up @@ -373,16 +373,20 @@ PyImport_AddModuleObject(PyObject *name)
return NULL;
}

// gh-86160: PyImport_AddModuleObject() returns a borrowed reference
PyObject *ref = PyWeakref_NewRef(mod, NULL);
// Convert to a borrowed reference
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Convert to a borrowed reference
// Convert to a borrowed reference.

Py_ssize_t old_refcnt = Py_REFCNT(mod);
Py_DECREF(mod);
if (ref == NULL) {
if (old_refcnt == 1) {
// gh-86160, gh-105922: The module got deleted. It can happen if
// sys.modules does not hold a strong reference to the module. For
// example, if sys.modules is not a regular dict but a custom type.
PyErr_SetString(PyExc_RuntimeError,
"sys.modules does not hold a strong reference "
"to the module");
return NULL;
}

mod = PyWeakref_GetObject(ref);
Py_DECREF(ref);
return mod; /* borrowed reference */
return mod; // borrowed reference, sys.modules holds a strong reference
}


Expand Down