GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||
GeezerVison is showing github.com — GH-135106: Restrict trashcan to GC'ed objects by markshannon · Pull Request #135682 · python/cpython Conversation
|
| 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.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
PyStackRef_CLOSE_SPECIALIZED calls Py_DECREF in the free threaded build. This is fixable, but requires more changes.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).Py_DECREF() on certain types of objects doesn't escape and won't invalidate borrowed references. One I encountered is: Line 9345 in 46c60e0
| if (key != NULL && PyDict_DelItem(subclasses, key)) { |
PyLongObject
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| } | ||
| /* 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.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I'll put the assert back.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
LGTM. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks @markshannon for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
(cherry picked from commit 2060089) Co-authored-by: Mark Shannon <[email protected]>
|
GH-135876 is a backport of this pull request to the 3.14 branch. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…5876) Co-authored-by: Mark Shannon <[email protected]>
…ythonGH-135682) (python#135876)" This reverts commit 46a7981.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.