gh-152298: Require owner claim for explicit lazy resolve

Author: 1sgtpepper

Include/internal/pycore_lazyimportobject.h

@@ -21,7 +21,7 @@ typedef struct {
     PyObject *lz_attr;
     // Protected by the import lock. lz_owner_* records the first module
     // global that resolve() may replace. Pending owners are being stored;
-    // pending and finalized may overlap during reentrant store resolution.
+    // finalized owners cannot be claimed again.
     PyObject *lz_owner_globals;
     PyObject *lz_owner_key;
     PyObject *lz_resolved;

Lib/test/test_lazy_import/__init__.py

@@ -753,7 +753,7 @@ def check():
         assert_python_ok("-c", code)
 
     @support.requires_subprocess()
-    def test_reentrant_store_resolution_clears_pending_owner(self):
+    def test_reentrant_store_resolution_waits_for_store_finish(self):
         code = textwrap.dedent("""
             import builtins
             import types
@@ -775,8 +775,11 @@ def custom_import(name, globals=None, locals=None, fromlist=None,
             class Reenter:
                 def __del__(self):
                     try:
-                        proxies.append(globals().copy()["target"])
-                        assert target.VALUE == "resolved"
+                        proxy = globals().copy()["target"]
+                        proxies.append(proxy)
+                        resolved = target
+                        assert resolved.VALUE == "resolved"
+                        assert globals()["target"] is proxy
                     except BaseException as exc:
                         errors.append((type(exc).__name__, str(exc)))
 
@@ -787,9 +790,12 @@ def __del__(self):
             try:
                 assert not errors, errors
                 assert len(proxies) == 1, proxies
-                resolved = globals()["target"]
                 proxy = proxies[0]
+                assert globals()["target"] is proxy
+
+                resolved = proxy.resolve()
                 assert resolved.VALUE == "resolved"
+                assert globals()["target"] is resolved
 
                 globals()["target"] = proxy
                 assert proxy.resolve() is resolved

Objects/lazyimportobject.c

@@ -179,6 +179,8 @@ clear_owner_if_matches(PyThreadState *tstate, PyLazyImportObject *m,
     _PyImport_AcquireLock(interp);
     if (owner_matches(m, globals, name)) {
         m->lz_owner_pending = 0;
+        // Keep lz_owner_finalized set: this owner has been committed or
+        // abandoned, so later stores of the same proxy are not canonical.
         Py_CLEAR(m->lz_owner_globals);
         Py_CLEAR(m->lz_owner_key);
     }
@@ -217,7 +219,10 @@ finalize_owner_if_matches(PyThreadState *tstate, PyLazyImportObject *m,
     int finalized = 0;
     PyInterpreterState *interp = tstate->interp;
     _PyImport_AcquireLock(interp);
-    if (!m->lz_owner_finalized && owner_matches(m, globals, name)) {
+    if (!m->lz_owner_pending &&
+        !m->lz_owner_finalized &&
+        owner_matches(m, globals, name))
+    {
         m->lz_owner_finalized = 1;
         finalized = 1;
     }
@@ -237,21 +242,45 @@ restore_owner_if_matches(PyThreadState *tstate, PyLazyImportObject *m,
     _PyImport_ReleaseLock(interp);
 }
 
-int
-_PyLazyImport_CommitIfCurrent(PyThreadState *tstate, PyObject *op,
-                              PyObject *globals, PyObject *name,
-                              PyObject *value)
+static int
+owner_is_pending(PyThreadState *tstate, PyLazyImportObject *m,
+                 PyObject *globals, PyObject *name)
 {
-    if (!PyLazyImport_CheckExact(op) ||
-        !PyDict_CheckExact(globals) ||
-        !PyUnicode_CheckExact(name))
-    {
-        return 0;
+    int pending = 0;
+    PyInterpreterState *interp = tstate->interp;
+    _PyImport_AcquireLock(interp);
+    if (m->lz_owner_pending && owner_matches(m, globals, name)) {
+        pending = 1;
     }
+    _PyImport_ReleaseLock(interp);
+    return pending;
+}
 
-    PyLazyImportObject *m = PyLazyImportObject_CAST(op);
-    int finalized_owner = finalize_owner_if_matches(tstate, m, globals, name);
+static int
+claim_stored_owner(PyThreadState *tstate, PyLazyImportObject *m,
+                   PyObject **globals, PyObject **name)
+{
+    int claimed = 0;
+    PyInterpreterState *interp = tstate->interp;
+    _PyImport_AcquireLock(interp);
+    if (!m->lz_owner_pending &&
+        !m->lz_owner_finalized &&
+        m->lz_owner_globals != NULL &&
+        m->lz_owner_key != NULL)
+    {
+        m->lz_owner_finalized = 1;
+        *globals = Py_NewRef(m->lz_owner_globals);
+        *name = Py_NewRef(m->lz_owner_key);
+        claimed = 1;
+    }
+    _PyImport_ReleaseLock(interp);
+    return claimed;
+}
 
+static int
+commit_if_current(PyObject *op, PyObject *globals, PyObject *name,
+                  PyObject *value)
+{
     PyObject *current = NULL;
     int err = 0;
     Py_BEGIN_CRITICAL_SECTION(globals);
@@ -265,7 +294,28 @@ _PyLazyImport_CommitIfCurrent(PyThreadState *tstate, PyObject *op,
     }
     Py_END_CRITICAL_SECTION();
     Py_XDECREF(current);
+    return err;
+}
 
+int
+_PyLazyImport_CommitIfCurrent(PyThreadState *tstate, PyObject *op,
+                              PyObject *globals, PyObject *name,
+                              PyObject *value)
+{
+    if (!PyLazyImport_CheckExact(op) ||
+        !PyDict_CheckExact(globals) ||
+        !PyUnicode_CheckExact(name))
+    {
+        return 0;
+    }
+
+    PyLazyImportObject *m = PyLazyImportObject_CAST(op);
+    if (owner_is_pending(tstate, m, globals, name)) {
+        return 0;
+    }
+
+    int finalized_owner = finalize_owner_if_matches(tstate, m, globals, name);
+    int err = commit_if_current(op, globals, name, value);
     if (finalized_owner) {
         if (err < 0) {
             restore_owner_if_matches(tstate, m, globals, name);
@@ -289,27 +339,19 @@ _PyLazyImport_CommitStoredIfCurrent(PyThreadState *tstate, PyObject *op,
     PyObject *globals = NULL;
     PyObject *name = NULL;
 
-    PyInterpreterState *interp = tstate->interp;
-    // Copy the owner under the import lock, then release it before entering a
-    // dict critical section in _PyLazyImport_CommitIfCurrent().
-    _PyImport_AcquireLock(interp);
-    if (!m->lz_owner_pending &&
-        !m->lz_owner_finalized &&
-        m->lz_owner_globals != NULL &&
-        m->lz_owner_key != NULL)
-    {
-        globals = Py_NewRef(m->lz_owner_globals);
-        name = Py_NewRef(m->lz_owner_key);
+    if (!claim_stored_owner(tstate, m, &globals, &name)) {
+        return 0;
     }
-    _PyImport_ReleaseLock(interp);
 
-    int err = 0;
-    if (globals != NULL && name != NULL) {
-        err = _PyLazyImport_CommitIfCurrent(
-            tstate, op, globals, name, value);
+    int err = commit_if_current(op, globals, name, value);
+    if (err < 0) {
+        restore_owner_if_matches(tstate, m, globals, name);
     }
-    Py_XDECREF(globals);
-    Py_XDECREF(name);
+    else {
+        clear_owner_if_matches(tstate, m, globals, name);
+    }
+    Py_DECREF(globals);
+    Py_DECREF(name);
     return err;
 }
 
@@ -345,7 +387,8 @@ PyDoc_STRVAR(lazy_import_doc,
 "\n"
 "A successful resolution is cached. Instances of this object accessed\n"
 "from the global scope will be automatically imported based upon their\n"
-"name and then replaced with the imported value.");
+"name. The original global is replaced only if it still refers to this\n"
+"lazy import object.");
 
 PyTypeObject PyLazyImport_Type = {
     PyVarObject_HEAD_INIT(&PyType_Type, 0)