Fatal assertion in PyObject_CallFinalizerFromDealloc()

[scoder]

Bug report

Bug description:

PyObject_CallFinalizerFromDealloc() starts with a fatal assertion:

cpython/Objects/object.c

Lines 591 to 627 in 6fcac09

	int
	PyObject_CallFinalizerFromDealloc(PyObject *self)
	{
	if (Py_REFCNT(self) != 0) {
	_PyObject_ASSERT_FAILED_MSG(self,
	"PyObject_CallFinalizerFromDealloc called "
	"on object with a non-zero refcount");
	}
	/* Temporarily resurrect the object. */
	_PyObject_ResurrectStart(self);
	PyObject_CallFinalizer(self);
	_PyObject_ASSERT_WITH_MSG(self,
	Py_REFCNT(self) > 0,
	"refcount is too small");
	_PyObject_ASSERT(self,
	(!_PyType_IS_GC(Py_TYPE(self))
	|| _PyObject_GC_IS_TRACKED(self)));
	/* Undo the temporary resurrection; can't use DECREF here, it would
	* cause a recursive call. */
	if (_PyObject_ResurrectEnd(self)) {
	/* tp_finalize resurrected it!
	gh-130202: Note that the object may still be dead in the free
	threaded build in some circumstances, so it's not safe to access
	`self` after this point. For example, the last reference to the
	resurrected `self` may be held by another thread, which can
	concurrently deallocate it. */
	return -1;
	}
	/* this is the normal path out, the caller continues with deallocation. */
	return 0;
	}

This was ok before FT-Python, but with freethreading concurrency, it can happen that an object gets resurrected by one thread before it even learns about its own finalisation in another thread. A fatal abort of the runtime seems very unhelpful in this case.

The fact that the function calls _PyObject_ResurrectStart() immediately after the assertion shows that the code is prepared for such a resurrection and could easily deal with it. Instead of the fatal assertion, it should check the refcount and abort the deallocation (by returning -1) if the refcount is higher than 0.

I noticed this issue while trying to port lxml to FT-Python. lxml uses unowned backlinks to Python objects from a C XML tree structure in order to assure a 1:1 mapping between C nodes and Python API proxy objects. The issues of the port are being discussed here:
lxml/lxml#477

CPython versions tested on:

3.14

Operating systems tested on:

No response

[scoder]

Here is a possible change:

diff --git a/Objects/object.c b/Objects/object.c
index fba86e63cd4..94e9ccfe3bf 100644
--- a/Objects/object.c
+++ b/Objects/object.c
@@ -592,9 +592,15 @@ int
 PyObject_CallFinalizerFromDealloc(PyObject *self)
 {
     if (Py_REFCNT(self) != 0) {
+        #ifdef Py_GIL_DISABLED
+        /* The object might have been resurrected concurrently
+           before we got here. Abort the deallocation. */
+        return -1;
+        #else
         _PyObject_ASSERT_FAILED_MSG(self,
                                     "PyObject_CallFinalizerFromDealloc called "
                                     "on object with a non-zero refcount");
+        #endif
     }
 
     /* Temporarily resurrect the object. */

This allows concurrent resurrections only in FT-Python. They are still most likely a user error otherwise.

Additionally, the allowed case could be restricted to refcount > 0 because a refcount < 0 is really a fatal error. But it's questionable whether this here is the right place to check that. Thus the simple change above.

[kumaraditya303]

This is a bug in implementation of lxml, it is not safe to incref a borrowed reference object directly as it is possible that is getting concurrently deallocated in another thread.

The bug lies in implementation of hasProxy, the correct code should be something like:

cdef inline bint hasProxy(xmlNode* c_node) noexcept:
    proxy = <cpython.object.PyObject*> c_node._private
    return (
        # Node has a proxy reference?
        proxy is not NULL and
        # Proxy reference is not currently deallocating?
        cpython.ref.PyUnstable_TryIncRef(proxy)
    )

If PyUnstable_TryIncRef succeeds then it is safe to use that proxy and that would also create a strong reference for the next getProxy call to consume.

Note that for PyUnstable_TryIncRef to work properly, the object should first be marked with PyUnstable_EnableTryIncRef.

[scoder]

Ah, cool. TryIncRef was what I was looking for but missed it from the bulk of recent additions. Thanks!