gh-158364: Don't report other interpreters' threads in sys._current_f… - #158369
Himesh-rupchandani wants to merge 1 commit into
Conversation
Documentation build overview
|
| .. versionchanged:: 3.16 | ||
| Only the threads of the current interpreter are included. Previously | ||
| the threads of other interpreters were reported as well, but the frame | ||
| objects created for them belonged to those interpreters, which could | ||
| corrupt the heap (each interpreter has its own memory arenas) and crash | ||
| the process. | ||
|
|
There was a problem hiding this comment.
I don't think this is worth documenting; nobody really cares that this used to crash.
| .. versionchanged:: 3.16 | ||
| Only the threads of the current interpreter are included, as for | ||
| :func:`_current_frames`. | ||
|
|
| # gh-158364: sys._current_frames() must not report the threads of | ||
| # other interpreters. A frame object materialized for such a thread | ||
| # belongs to that interpreter, and since each interpreter has its own | ||
| # memory arenas, it would be deallocated by a different interpreter | ||
| # than the one that allocated it, corrupting the heap. |
There was a problem hiding this comment.
This comment is really long. Let's just say something like gh-158364: sys._current_frames() would access frames of another interpreter and crash
| # than the one that allocated it, corrupting the heap. | ||
| import threading | ||
|
|
||
| # Park a thread of *this* interpreter at a known place. |
There was a problem hiding this comment.
Not necessary.
| # Park a thread of *this* interpreter at a known place. |
| entered_g = threading.Event() | ||
| leave_g = threading.Event() |
There was a problem hiding this comment.
What is the _g suffix? Let's just call these entered and left (leave is present tense).
|
|
||
| _Py_EnsureTstateNotNULL(tstate); | ||
|
|
There was a problem hiding this comment.
This has a non-zero cost, since it performs an actual NULL check at runtime. It's also not related to this fix, so let's remove it:
| _Py_EnsureTstateNotNULL(tstate); |
| * | ||
| * Threads of other interpreters are not included (gh-158364). A frame | ||
| * object materialized for such a thread belongs to that interpreter: it | ||
| * is stored in its _PyInterpreterFrame.frame_obj and deallocated when | ||
| * that interpreter pops the frame, while the caller drops the reference | ||
| * held by the returned dict at some arbitrary later time. Since PEP 684 | ||
| * gives each interpreter its own obmalloc arenas, the block can be freed | ||
| * by a different interpreter than the one that allocated it, corrupting | ||
| * the heap. Handing out such a frame is unsafe for other reasons too: it | ||
| * holds references to the other interpreter's objects, and that | ||
| * interpreter may be finalized while the caller still holds the frame. | ||
| * Code running in each interpreter sees its own threads. |
There was a problem hiding this comment.
This comment isn't helpful to anyone reading this in the future. If all we did was delete code, there's no reason to tell them that.
| * | ||
| * Threads of other interpreters are not included, for the same reason as | ||
| * in _PyThread_CurrentFrames(): the returned dict would hold a reference | ||
| * to an exception owned by another interpreter, which can then be | ||
| * deallocated by whichever interpreter drops it last (gh-158364). |
There was a problem hiding this comment.
Same thing here. Let's get rid of this.
| Fix heap corruption (typically an abort or crash in the C library's | ||
| ``free()``) when :func:`sys._current_frames` or | ||
| :func:`sys._current_exceptions` is called while another interpreter is | ||
| running. Both functions no longer report the threads of other | ||
| interpreters: a frame object materialized for such a thread belongs to | ||
| that interpreter, and since each interpreter has its own memory arenas it | ||
| was being deallocated by a different interpreter than the one that | ||
| allocated it. |
There was a problem hiding this comment.
This contains a lot of technical information that isn't relevant to changelog readers. We need to say what was fixed, not how:
| Fix heap corruption (typically an abort or crash in the C library's | |
| ``free()``) when :func:`sys._current_frames` or | |
| :func:`sys._current_exceptions` is called while another interpreter is | |
| running. Both functions no longer report the threads of other | |
| interpreters: a frame object materialized for such a thread belongs to | |
| that interpreter, and since each interpreter has its own memory arenas it | |
| was being deallocated by a different interpreter than the one that | |
| allocated it. | |
| Fix crash when :func:`sys._current_frames` or | |
| :func:`sys._current_exceptions` is called while another interpreter is | |
| running. |
| interp.exec(textwrap.dedent(f''' | ||
| import sys |
There was a problem hiding this comment.
Up to you, but it's usually easier to just do if True: instead of textwrap.dedent.
|
Thanks for the review! Force-pushed with all of it addressed: Both versionchanged blocks are gone — Doc/library/sys.rst is untouched now. |
|
A thread only shows up in sys._current_exceptions() with something other |
f10e587 to
e05c195
Compare
…rent_frames() sys._current_frames() materialized a PyFrameObject for every thread of every interpreter. A frame object created for a thread of another interpreter belongs to that interpreter: it is stored in its _PyInterpreterFrame.frame_obj and deallocated when that interpreter pops the frame, while the calling interpreter holds the reference from the returned dict and drops it at some arbitrary later time. Since PEP 684 gives each interpreter its own obmalloc arenas, the block ends up being freed by a different interpreter than the one that allocated it, which corrupts the heap and typically aborts the process inside free(). sys._current_exceptions() has the same problem: it hands out references to exception objects owned by other interpreters. Both functions now only report the threads of the calling interpreter. This matches PyUnstable_DumpTracebackThreads() (used by faulthandler), which already only dumps the threads of one interpreter. As a consequence, only the current interpreter's world needs to be stopped.
e05c195 to
20ce55f
Compare
2 participants
…rames()
sys._current_frames() materialized a PyFrameObject for every thread of every interpreter. A frame object created for a thread of another interpreter belongs to that interpreter: it is stored in its _PyInterpreterFrame.frame_obj and deallocated when that interpreter pops the frame, while the calling interpreter holds the reference from the returned dict and drops it at some arbitrary later time. Since PEP 684 gives each interpreter its own obmalloc arenas, the block ends up being freed by a different interpreter than the one that allocated it, which corrupts the heap and typically aborts the process inside free().
sys._current_exceptions() has the same problem: it hands out references to exception objects owned by other interpreters.
Both functions now only report the threads of the calling interpreter. This matches PyUnstable_DumpTracebackThreads() (used by faulthandler), which already only dumps the threads of one interpreter. As a consequence, only the current interpreter's world needs to be stopped.