[3.13] gh-119560: Drop an Invalid Assert in PyState_FindModule() (gh-119561) (gh-119632)

miss-islington · ericsnowcurrently · web-flow · commit bd9983cab89c · 2024-05-27T19:11:29.000Z
The assertion was added in gh-118532 but was based on the invalid assumption that PyState_FindModule() would only be called with an already-initialized module def. I've added a test to make sure we don't make that assumption again. (cherry picked from commit 0c5ebe1) Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py
@@ -2887,6 +2887,13 @@ def test_with_reinit_reloaded(self):
 
                 self.assertIs(reloaded.snapshot.cached, reloaded.module)
 
+    def test_check_state_first(self):
+        for variant in ['', '_with_reinit', '_with_state']:
+            name = f'{self.NAME}{variant}_check_cache_first'
+            with self.subTest(name):
+                mod = self._load_dynamic(name, self.ORIGIN)
+                self.assertEqual(mod.__name__, name)
+
     # Currently, for every single-phrase init module loaded
     # in multiple interpreters, those interpreters share a
     # PyModuleDef for that object, which can be a problem.
diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-05-25-12-52-25.gh-issue-119560.wSlm8q.rst b/Misc/NEWS.d/next/Core and Builtins/2024-05-25-12-52-25.gh-issue-119560.wSlm8q.rst
@@ -0,0 +1,3 @@
+An invalid assert in beta 1 has been removed.  The assert would fail if
+``PyState_FindModule()`` was used in an extension module's init function
+before the module def had been initialized.
diff --git a/Modules/_testsinglephase.c b/Modules/_testsinglephase.c
@@ -1,7 +1,7 @@
 
 /* Testing module for single-phase initialization of extension modules
 
-This file contains 5 distinct modules, meaning each as its own name
+This file contains 8 distinct modules, meaning each as its own name
 and its own init function (PyInit_...).  The default import system will
 only find the one matching the filename: _testsinglephase.  To load the
 others you must do so manually.  For example:
@@ -14,7 +14,7 @@ spec = importlib.util.spec_from_file_location(name, filename, loader=loader)
 mod = importlib._bootstrap._load(spec)
 ```
 
-Here are the 5 modules:
+Here are the 8 modules:
 
 * _testsinglephase
    * def: _testsinglephase_basic,
@@ -136,6 +136,32 @@ Here are the 5 modules:
       5. increment <global>.initialized_count
    * functions: see common functions below
    * import system: same as _testsinglephase_basic_copy
+* _testsinglephase_check_cache_first
+   * def: _testsinglepahse_check_cache_first
+      * m_name: "_testsinglephase_check_cache_first"
+      * m_size: -1
+   * state: none
+   * init function:
+      * tries PyState_FindModule() first
+      * otherwise creates empty module
+   * functions: none
+   * import system: same as _testsinglephase
+* _testsinglephase_with_reinit_check_cache_first
+   * def: _testsinglepahse_with_reinit_check_cache_first
+      * m_name: "_testsinglephase_with_reinit_check_cache_first"
+      * m_size: 0
+   * state: none
+   * init function: same as _testsinglephase_check_cache_first
+   * functions: none
+   * import system: same as _testsinglephase_with_reinit
+* _testsinglephase_with_state_check_cache_first
+   * def: _testsinglepahse_with_state_check_cache_first
+      * m_name: "_testsinglephase_with_state_check_cache_first"
+      * m_size: 42
+   * state: none
+   * init function: same as _testsinglephase_check_cache_first
+   * functions: none
+   * import system: same as _testsinglephase_with_state
 
 Module state:
 
@@ -650,3 +676,64 @@ PyInit__testsinglephase_with_state(void)
 finally:
     return module;
 }
+
+
+/****************************************************/
+/* the _testsinglephase_*_check_cache_first modules */
+/****************************************************/
+
+static struct PyModuleDef _testsinglephase_check_cache_first = {
+    PyModuleDef_HEAD_INIT,
+    .m_name = "_testsinglephase_check_cache_first",
+    .m_doc = PyDoc_STR("Test module _testsinglephase_check_cache_first"),
+    .m_size = -1,  // no module state
+};
+
+PyMODINIT_FUNC
+PyInit__testsinglephase_check_cache_first(void)
+{
+    assert(_testsinglephase_check_cache_first.m_base.m_index == 0);
+    PyObject *mod = PyState_FindModule(&_testsinglephase_check_cache_first);
+    if (mod != NULL) {
+        return Py_NewRef(mod);
+    }
+    return PyModule_Create(&_testsinglephase_check_cache_first);
+}
+
+
+static struct PyModuleDef _testsinglephase_with_reinit_check_cache_first = {
+    PyModuleDef_HEAD_INIT,
+    .m_name = "_testsinglephase_with_reinit_check_cache_first",
+    .m_doc = PyDoc_STR("Test module _testsinglephase_with_reinit_check_cache_first"),
+    .m_size = 0,  // no module state
+};
+
+PyMODINIT_FUNC
+PyInit__testsinglephase_with_reinit_check_cache_first(void)
+{
+    assert(_testsinglephase_with_reinit_check_cache_first.m_base.m_index == 0);
+    PyObject *mod = PyState_FindModule(&_testsinglephase_with_reinit_check_cache_first);
+    if (mod != NULL) {
+        return Py_NewRef(mod);
+    }
+    return PyModule_Create(&_testsinglephase_with_reinit_check_cache_first);
+}
+
+
+static struct PyModuleDef _testsinglephase_with_state_check_cache_first = {
+    PyModuleDef_HEAD_INIT,
+    .m_name = "_testsinglephase_with_state_check_cache_first",
+    .m_doc = PyDoc_STR("Test module _testsinglephase_with_state_check_cache_first"),
+    .m_size = 42,  // not used
+};
+
+PyMODINIT_FUNC
+PyInit__testsinglephase_with_state_check_cache_first(void)
+{
+    assert(_testsinglephase_with_state_check_cache_first.m_base.m_index == 0);
+    PyObject *mod = PyState_FindModule(&_testsinglephase_with_state_check_cache_first);
+    if (mod != NULL) {
+        return Py_NewRef(mod);
+    }
+    return PyModule_Create(&_testsinglephase_with_state_check_cache_first);
+}
diff --git a/Python/import.c b/Python/import.c
@@ -457,7 +457,6 @@ static Py_ssize_t
 _get_module_index_from_def(PyModuleDef *def)
 {
     Py_ssize_t index = def->m_base.m_index;
-    assert(index > 0);
 #ifndef NDEBUG
     struct extensions_cache_value *cached = _find_cached_def(def);
     assert(cached == NULL || index == _get_cached_module_index(cached));
@@ -489,7 +488,7 @@ _set_module_index(PyModuleDef *def, Py_ssize_t index)
 static const char *
 _modules_by_index_check(PyInterpreterState *interp, Py_ssize_t index)
 {
-    if (index == 0) {
+    if (index <= 0) {
         return "invalid module index";
     }
     if (MODULES_BY_INDEX(interp) == NULL) {

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+An invalid assert in beta 1 has been removed. The assert would fail if`
	`2`	+``PyState_FindModule()`` was used in an extension module's init function
	`3`	`+before the module def had been initialized.`
Original file line number	Diff line number	Diff line change
`@@ -457,7 +457,6 @@ static Py_ssize_t`
`457`	`457`	`_get_module_index_from_def(PyModuleDef *def)`
`458`	`458`	`{`
`459`	`459`	`Py_ssize_t index = def->m_base.m_index;`
`460`		`- assert(index > 0);`
`461`	`460`	`#ifndef NDEBUG`
`462`	`461`	`struct extensions_cache_value *cached = _find_cached_def(def);`
`463`	`462`	`assert(cached == NULL \|\| index == _get_cached_module_index(cached));`
`@@ -489,7 +488,7 @@ _set_module_index(PyModuleDef *def, Py_ssize_t index)`
`489`	`488`	`static const char *`
`490`	`489`	`_modules_by_index_check(PyInterpreterState *interp, Py_ssize_t index)`
`491`	`490`	`{`
`492`		`- if (index == 0) {`
	`491`	`+ if (index <= 0) {`
`493`	`492`	`return "invalid module index";`
`494`	`493`	`}`
`495`	`494`	`if (MODULES_BY_INDEX(interp) == NULL) {`