Skip to content
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.

Sign up for GitHub

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

Jump to bottom

gh-106092: Fix use-after-free crash in frame_dealloc #106875

Merged
merged 1 commit into from
Aug 1, 2023
Merged

gh-106092: Fix use-after-free crash in frame_dealloc #106875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2023-07-18-16-13-51.gh-issue-106092.bObgRM.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix a segmentation fault caused by a use-after-free bug in ``frame_dealloc``
when the trashcan delays the deallocation of a ``PyFrameObject``.
13 changes: 7 additions & 6 deletions Objects/frameobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -879,20 +879,21 @@ frame_dealloc(PyFrameObject *f)
/* It is the responsibility of the owning generator/coroutine
* to have cleared the generator pointer */

assert(f->f_frame->owner != FRAME_OWNED_BY_GENERATOR ||
_PyFrame_GetGenerator(f->f_frame)->gi_frame_state == FRAME_CLEARED);

if (_PyObject_GC_IS_TRACKED(f)) {
_PyObject_GC_UNTRACK(f);
}

Py_TRASHCAN_BEGIN(f, frame_dealloc);
PyObject *co = NULL;

/* GH-106092: If f->f_frame was on the stack and we reached the maximum
* nesting depth for deallocations, the trashcan may have delayed this
* deallocation until after f->f_frame is freed. Avoid dereferencing
* f->f_frame unless we know it still points to valid memory. */
_PyInterpreterFrame *frame = (_PyInterpreterFrame *)f->_f_frame_data;

/* Kill all local variables including specials, if we own them */
if (f->f_frame->owner == FRAME_OWNED_BY_FRAME_OBJECT) {
assert(f->f_frame == (_PyInterpreterFrame *)f->_f_frame_data);
_PyInterpreterFrame *frame = (_PyInterpreterFrame *)f->_f_frame_data;
if (f->f_frame == frame && frame->owner == FRAME_OWNED_BY_FRAME_OBJECT) {
Copy link
Member

Choose a reason for hiding this comment

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

This looks correct.

Note:
In theory, f->f_frame == frame and frame->owner == FRAME_OWNED_BY_FRAME_OBJECT should be equivalent, but in practice they are not.
We can't just check frame->owner == FRAME_OWNED_BY_FRAME_OBJECT because of the Trashcan mechanism, and we can't just check f->f_frame == frame because of this.
So we need both checks here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this check be put in other places too? We are currently investigating the root cause of segfaults in 3.11.5 with call stacks ending with

#0  0x0000000000635d9a in ?? ()
#1  0x00000000004d8e27 in PyFrame_FastToLocalsWithError ()
#2  0x00000000004d8e3e in ?? ()
#3  0x000000000054e5f6 in _PyObject_GenericGetAttrWithDict ()

What we are trying to access here is frame.f_locals. The frame object is obtained from looping over all thread states and calling PyThreadState_GetFrame on them (so we know we have a strong reference to it). Because of the inlining it's not immediately obvious where in PyFrame_FastToLocalsWithError we are segfaulting exactly, but our current hunch is that it is because the referenced _PyInterpreterState might have been deallocated. We're still investigating this.

/* Don't clear code object until the end */
co = frame->f_executable;
frame->f_executable = NULL;
Expand Down
Loading