Skip to content

GH-135106: Restrict trashcan to GC'ed objects #135682

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

Merged
merged 5 commits into from
Jun 24, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Restrict the trashcan mechanism to GC'ed objects and untrack them while in
the trashcan to prevent the GC and trashcan mechanisms conflicting.
88 changes: 29 additions & 59 deletions Objects/object.c
Original file line number Diff line number Diff line change
Expand Up @@ -3034,57 +3034,24 @@ 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);
#if SIZEOF_VOID_P > 4
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;
#else
// Make the top two bits 0, so it appears mortal.
return (full >> 2) + 1;
#endif
}

static void *
safe_refcount_to_pointer(uintptr_t refcnt)
{
#if SIZEOF_VOID_P > 4
if (refcnt & 1) {
refcnt += _Py_IMMORTAL_MINIMUM_REFCNT - 1;
}
return (void *)(refcnt - 2);
#else
return (void *)((refcnt -1) << 2);
#endif
}
#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.
* call-stack depth gets large. op must be a gc'ed object, with refcount 0.
* Py_DECREF must already have been called on it.
*/
void
_PyTrash_thread_deposit_object(PyThreadState *tstate, PyObject *op)
{
_PyObject_ASSERT(op, Py_REFCNT(op) == 0);
assert(PyObject_IS_GC(op));
int tracked = _PyObject_GC_IS_TRACKED(op);
if (tracked) {
_PyObject_GC_UNTRACK(op);
}
uintptr_t tagged_ptr = ((uintptr_t)tstate->delete_later) | tracked;
#ifdef Py_GIL_DISABLED
op->ob_tid = (uintptr_t)tstate->delete_later;
op->ob_tid = tagged_ptr;
#else
/* 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));
_Py_AS_GC(op)->_gc_next = tagged_ptr;
#endif
tstate->delete_later = op;
}
Expand All @@ -3099,25 +3066,28 @@ _PyTrash_thread_destroy_chain(PyThreadState *tstate)
destructor dealloc = Py_TYPE(op)->tp_dealloc;

#ifdef Py_GIL_DISABLED
tstate->delete_later = (PyObject*) op->ob_tid;
uintptr_t tagged_ptr = op->ob_tid;
op->ob_tid = 0;
_Py_atomic_store_ssize_relaxed(&op->ob_ref_shared, _Py_REF_MERGED);
#else
/* Get the delete_later pointer from the refcnt field.
* See _PyTrash_thread_deposit_object(). */
uintptr_t refcnt = *((uintptr_t*)op);
tstate->delete_later = safe_refcount_to_pointer(refcnt);
op->ob_refcnt = 0;
uintptr_t tagged_ptr = _Py_AS_GC(op)->_gc_next;
_Py_AS_GC(op)->_gc_next = 0;
#endif

/* Call the deallocator directly. This used to try to
* fool Py_DECREF into calling it indirectly, but
* Py_DECREF was already called on this object, and in
* assorted non-release builds calling Py_DECREF again ends
* up distorting allocation statistics.
*/
_PyObject_ASSERT(op, Py_REFCNT(op) == 0);
(*dealloc)(op);
tstate->delete_later = (PyObject *)(tagged_ptr & ~1);
if (tagged_ptr & 1) {
_PyObject_GC_TRACK(op);
}
/* It is possible that the object has been accessed through
* a weak ref, so only free if refcount == 0) */
if (Py_REFCNT(op) == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you already brought this up in the meeting, but I don't think this should be necessary. Weakrefs won't resurrect an object with refcount of zero, which _PyTrash_thread_deposit_object() ensures.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll put the assert back.

/* Call the deallocator directly. This used to try to
* fool Py_DECREF into calling it indirectly, but
* Py_DECREF was already called on this object, and in
* assorted non-release builds calling Py_DECREF again ends
* up distorting allocation statistics.
*/
(*dealloc)(op);
}
}
}

Expand Down Expand Up @@ -3186,7 +3156,7 @@ _Py_Dealloc(PyObject *op)
destructor dealloc = type->tp_dealloc;
PyThreadState *tstate = _PyThreadState_GET();
intptr_t margin = _Py_RecursionLimit_GetMargin(tstate);
if (margin < 2) {
if (margin < 2 && PyObject_IS_GC(op)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you also need to guard the _PyTrash_thread_destroy_chain() below; otherwise destructors for simple types (like PyLongObject) can trigger arbitrary code execution.

That checks seems a bit more difficult to add efficiently.

Copy link
Member Author

Choose a reason for hiding this comment

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

otherwise destructors for simple types (like PyLongObject) can trigger arbitrary code execution.

Does that matter?
We only care that PyStackRef_CLOSE_SPECIALIZED doesn't escape and that doesn't (at least, shouldn't) call Py_DECREF.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. PyStackRef_CLOSE_SPECIALIZED calls Py_DECREF in the free threaded build. This is fixable, but requires more changes.
  2. obj.attr += 1 wouldn't be atomic in the GIL-enabled build when using a lot of stack (already not atomic in free threaded build).
  3. I think there are places in C code where we assume that Py_DECREF() on certain types of objects doesn't escape and won't invalidate borrowed references. One I encountered is:
    if (key != NULL && PyDict_DelItem(subclasses, key)) {
    where key is a PyLongObject

Copy link
Member Author

Choose a reason for hiding this comment

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

We can do it reasonably efficiently with a read of tp_flags. Since we are reading tp_dealloc anyway, the latency is likely hidden.

_PyTrash_thread_deposit_object(tstate, (PyObject *)op);
return;
}
Expand Down
Loading