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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Jun 18, 2025

Objects/object.c Outdated
@@ -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.

Objects/object.c Outdated
}
/* 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.

@markshannon markshannon marked this pull request as ready for review June 19, 2025 13:59
@markshannon markshannon added the needs backport to 3.14 bugs and security fixes label Jun 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants