GH-133261: Make sure that the GC doesn't untrack objects in trashcan (GH-133431)

markshannon · web-flow · commit f554237b8ef6 · 2025-05-05T13:44:50.000+01:00
diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2025-05-05-12-03-46.gh-issue-133261.bL1gqz.rst b/Misc/NEWS.d/next/Core_and_Builtins/2025-05-05-12-03-46.gh-issue-133261.bL1gqz.rst
@@ -0,0 +1,3 @@
+Fix bug where the cycle GC could untrack objects in the trashcan because
+they looked like they were immortal. When objects are added to the trashcan,
+we take care to ensure they keep a mortal reference count.
diff --git a/Objects/object.c b/Objects/object.c
@@ -2930,6 +2930,33 @@ Py_ReprLeave(PyObject *obj)
 
 /* Trashcan support. */
 
+#ifndef Py_GIL_DISABLED
+/* We need to store a pointer in the refcount field of
+ * an object. It is important that we never store 0 (NULL).
+* It is also important to not make the object appear immortal,
+* or it might be untracked by the cycle GC. */
+static uintptr_t
+pointer_to_safe_refcount(void *ptr)
+{
+    uintptr_t full = (uintptr_t)ptr;
+    assert((full & 3) == 0);
+    uint32_t refcnt = (uint32_t)full;
+    if (refcnt >= (uint32_t)_Py_IMMORTAL_MINIMUM_REFCNT) {
+        full = full - ((uintptr_t)_Py_IMMORTAL_MINIMUM_REFCNT) + 1;
+    }
+    return full + 2;
+}
+
+static void *
+safe_refcount_to_pointer(uintptr_t refcnt)
+{
+    if (refcnt & 1) {
+        refcnt += _Py_IMMORTAL_MINIMUM_REFCNT - 1;
+    }
+    return (void *)(refcnt - 2);
+}
+#endif
+
 /* Add op to the gcstate->trash_delete_later list.  Called when the current
  * call-stack depth gets large.  op must be a currently untracked gc'ed
  * object, with refcount 0.  Py_DECREF must already have been called on it.
@@ -2941,11 +2968,10 @@ _PyTrash_thread_deposit_object(PyThreadState *tstate, PyObject *op)
 #ifdef Py_GIL_DISABLED
     op->ob_tid = (uintptr_t)tstate->delete_later;
 #else
-    /* Store the delete_later pointer in the refcnt field.
-     * As this object may still be tracked by the GC,
-     * it is important that we never store 0 (NULL). */
-    uintptr_t refcnt = (uintptr_t)tstate->delete_later;
-    *((uintptr_t*)op) = refcnt+1;
+    /* Store the delete_later pointer in the refcnt field. */
+    uintptr_t refcnt = pointer_to_safe_refcount(tstate->delete_later);
+    *((uintptr_t*)op) = refcnt;
+    assert(!_Py_IsImmortal(op));
 #endif
     tstate->delete_later = op;
 }
@@ -2967,7 +2993,7 @@ _PyTrash_thread_destroy_chain(PyThreadState *tstate)
         /* Get the delete_later pointer from the refcnt field.
          * See _PyTrash_thread_deposit_object(). */
         uintptr_t refcnt = *((uintptr_t*)op);
-        tstate->delete_later = (PyObject *)(refcnt - 1);
+        tstate->delete_later = safe_refcount_to_pointer(refcnt);
         op->ob_refcnt = 0;
 #endif
 
diff --git a/Python/gc.c b/Python/gc.c
@@ -493,6 +493,7 @@ update_refs(PyGC_Head *containers)
         next = GC_NEXT(gc);
         PyObject *op = FROM_GC(gc);
         if (_Py_IsImmortal(op)) {
+            assert(!_Py_IsStaticImmortal(op));
             _PyObject_GC_UNTRACK(op);
            gc = next;
            continue;

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+Fix bug where the cycle GC could untrack objects in the trashcan because`
	`2`	`+they looked like they were immortal. When objects are added to the trashcan,`
	`3`	`+we take care to ensure they keep a mortal reference count.`