Conversation
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)) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
PyStackRef_CLOSE_SPECIALIZEDcallsPy_DECREFin the free threaded build. This is fixable, but requires more changes.obj.attr += 1wouldn't be atomic in the GIL-enabled build when using a lot of stack (already not atomic in free threaded build).- 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:where key is aPyLongObject
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'll put the assert back.
… and thread the trashcan list through the GC header
test_weakref.test_threaded_weak_key_dict_deepcopycrash: merged objects should have ob_tid == 0 #135106