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

[andersk]

It was possible for the trashcan to delay the deallocation of a PyFrameObject until after its corresponding _PyInterpreterFrame has already been freed. So frame_dealloc needs to avoid dereferencing the f_frame pointer unless it first checks that the pointer still points to valid memory.

Fixes #106092. This should be backported to 3.11 and 3.12.

• Issue: Segmentation fault in 3.11.4, 3.12.0b3; _PyInterpreterFrame ownership issue #106092

[markshannon]

I think it better to ensure that f->f_frame always points to valid memory, rather than do the additional check.
Something like https://github.com/python/cpython/compare/main...faster-cpython:cpython:safe-clear-frame?expand=1

Do you have a reproducer that doesn't need third-party modules?

[andersk]

Something like https://github.com/python/cpython/compare/main...faster-cpython:cpython:safe-clear-frame?expand=1

+static const _PyInterpreterFrame cleared_frame = {
+    .owner = FRAME_CLEARED
+};

Note that FRAME_CLEARED has never been a valid member of enum _frameowner; see #106156.

+        f->f_frame = (_PyInterpreterFrame *)&cleared_frame;
         Py_DECREF(f);

Couldn’t that incorrectly skip the deallocation of f_executable, f_funcobj, and f_locals in frame_dealloc, if the frame was allocated by PyFrame_New and has owner == FRAME_OWNED_BY_FRAME_OBJECT?

Do you have a reproducer that doesn't need third-party modules?

Not at present—further minimization is probably possible but difficult, because the segfault requires a specific coincidence of the position of the stack pointer within the current stack chunk, the GC threshold, and the trashcan recursion depth.

[markshannon]

A reproducer that causes an assertion failure would be good enough, I think.

[markshannon]

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.

[P403n1x87]

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.

[miss-islington]

Thanks @andersk for the PR, and @markshannon for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

[bedevere-bot]

GH-107532 is a backport of this pull request to the 3.12 branch.

[miss-islington]

Thanks @andersk for the PR, and @markshannon for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

[bedevere-bot]

GH-107533 is a backport of this pull request to the 3.11 branch.