Skip to content

gh-116098: Clean up frame allocation code and remove the invalid sneaky frame test #116687

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.

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

Merged
merged 2 commits into from
Mar 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
65 changes: 0 additions & 65 deletions Lib/test/test_frame.py
Original file line number Diff line number Diff line change
Expand Up @@ -293,71 +293,6 @@ def gen():
""")
assert_python_ok("-c", code)

@support.cpython_only
@unittest.skipIf(Py_GIL_DISABLED, "test requires precise GC scheduling")
def test_sneaky_frame_object(self):

def trace(frame, event, arg):
"""
Don't actually do anything, just force a frame object to be created.
"""

def callback(phase, info):
"""
Yo dawg, I heard you like frames, so I'm allocating a frame while
you're allocating a frame, so you can have a frame while you have a
frame!
"""
nonlocal sneaky_frame_object
sneaky_frame_object = sys._getframe().f_back.f_back
# We're done here:
gc.callbacks.remove(callback)

def f():
while True:
yield

old_threshold = gc.get_threshold()
old_callbacks = gc.callbacks[:]
old_enabled = gc.isenabled()
old_trace = sys.gettrace()
try:
# Stop the GC for a second while we set things up:
gc.disable()
# Create a paused generator:
g = f()
next(g)
# Move all objects to the oldest generation, and tell the GC to run
# on the *very next* allocation:
gc.collect()
gc.set_threshold(1, 0, 0)
sys._clear_internal_caches()
# Okay, so here's the nightmare scenario:
# - We're tracing the resumption of a generator, which creates a new
# frame object.
# - The allocation of this frame object triggers a collection
# *before* the frame object is actually created.
# - During the collection, we request the exact same frame object.
# This test does it with a GC callback, but in real code it would
# likely be a trace function, weakref callback, or finalizer.
# - The collection finishes, and the original frame object is
# created. We now have two frame objects fighting over ownership
# of the same interpreter frame!
sys.settrace(trace)
gc.callbacks.append(callback)
sneaky_frame_object = None
gc.enable()
next(g)
# g.gi_frame should be the frame object from the callback (the
# one that was *requested* second, but *created* first):
self.assertIs(g.gi_frame, sneaky_frame_object)
finally:
gc.set_threshold(*old_threshold)
gc.callbacks[:] = old_callbacks
sys.settrace(old_trace)
if old_enabled:
gc.enable()

@support.cpython_only
@threading_helper.requires_working_threading()
def test_sneaky_frame_object_teardown(self):
Expand Down
27 changes: 9 additions & 18 deletions Python/frame.c
Original file line number Diff line number Diff line change
Expand Up @@ -37,24 +37,15 @@ _PyFrame_MakeAndSetFrameObject(_PyInterpreterFrame *frame)
return NULL;
}
PyErr_SetRaisedException(exc);
if (frame->frame_obj) {
// GH-97002: How did we get into this horrible situation? Most likely,
// allocating f triggered a GC collection, which ran some code that
// *also* created the same frame... while we were in the middle of
// creating it! See test_sneaky_frame_object in test_frame.py for a
// concrete example.
//
// Regardless, just throw f away and use that frame instead, since it's
// already been exposed to user code. It's actually a bit tricky to do
// this, since we aren't backed by a real _PyInterpreterFrame anymore.
// Just pretend that we have an owned, cleared frame so frame_dealloc
// doesn't make the situation worse:
f->f_frame = (_PyInterpreterFrame *)f->_f_frame_data;
f->f_frame->owner = FRAME_CLEARED;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also remove the corresponding assert(frame->owner != FRAME_CLEARED) statements, especially since FRAME_CLEARED was never a valid member of enum _frameowner (it’s from enum _framestate).

f->f_frame->frame_obj = f;
Py_DECREF(f);
return frame->frame_obj;
}

// GH-97002: There was a time when a frame object could be created when we
// are allocating the new frame object f above, so frame->frame_obj would
// be assigned already. That path does not exist anymore. We won't call any
// Python code in this function and garbage collection will not run.
// Notice that _PyFrame_New_NoTrack() can potentially raise a MemoryError,
// but it won't allocate a traceback until the frame unwinds, so we are safe
// here.
assert(frame->frame_obj == NULL);
assert(frame->owner != FRAME_OWNED_BY_FRAME_OBJECT);
assert(frame->owner != FRAME_CLEARED);
f->f_frame = frame;
Expand Down