Skip to content
Open
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
7 changes: 5 additions & 2 deletions Include/internal/pycore_code.h
Original file line number Diff line number Diff line change
Expand Up @@ -566,8 +566,11 @@ _PyCode_GetTLBCFast(PyThreadState *tstate, PyCodeObject *co)
{
_PyCodeArray *code = _PyCode_GetTLBCArray(co);
int32_t idx = ((_PyThreadStateImpl*) tstate)->tlbc_index;
if (idx < code->size && code->entries[idx] != NULL) {
return (_Py_CODEUNIT *) code->entries[idx];
if (idx < code->size) {
void *entry = _Py_atomic_load_ptr_acquire(&code->entries[idx]);
if (entry != NULL) {
return (_Py_CODEUNIT *) entry;
}
}
return NULL;
}
Expand Down
8 changes: 7 additions & 1 deletion Include/internal/pycore_interpframe.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ _PyFrame_GetBytecode(_PyInterpreterFrame *f)
PyCodeObject *co = _PyFrame_GetCode(f);
_PyCodeArray *tlbc = _PyCode_GetTLBCArray(co);
assert(f->tlbc_index >= 0 && f->tlbc_index < tlbc->size);
return (_Py_CODEUNIT *)tlbc->entries[f->tlbc_index];
return (_Py_CODEUNIT *)_Py_atomic_load_ptr_acquire(&tlbc->entries[f->tlbc_index]);
#else
return _PyCode_CODE(_PyFrame_GetCode(f));
#endif
Expand Down Expand Up @@ -294,6 +294,12 @@ _PyFrame_IsIncomplete(_PyInterpreterFrame *frame)
if (frame->owner >= FRAME_OWNED_BY_INTERPRETER) {
return true;
}
/* Frames owned by a frame object are guaranteed complete by take_ownership().
* Checking instr_ptr here would race with take_ownership() when called from
* another thread (e.g. pdb walking frame->f_back cross-thread). */
if (frame->owner == FRAME_OWNED_BY_FRAME_OBJECT) {
return false;
}
return frame->owner != FRAME_OWNED_BY_GENERATOR &&
frame->instr_ptr < _PyFrame_GetBytecode(frame) +
_PyFrame_GetCode(frame)->_co_firsttraceable;
Expand Down
34 changes: 34 additions & 0 deletions Lib/test/test_free_threading/test_frame.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,40 @@ def clearer():

threading_helper.run_concurrently([reader, reader, clearer])

def test_f_back_after_thread_return(self):
# gh-149110: Accessing frame.f_back cross-thread while the owning
# thread is returning caused a spurious assertion failure in
# PyFrame_GetBack on free-threaded builds.
frames = []
lock = threading.Lock()

def target():
frame = sys._getframe()
with lock:
frames.append(frame)

def inspector():
for _ in range(50):
frame = None
with lock:
if frames:
frame = frames[-1]
if frame is not None:
try:
_ = frame.f_back
except Exception:
pass
sys._clear_internal_caches()

threads = [threading.Thread(target=target) for _ in range(20)]
insp = threading.Thread(target=inspector)
insp.start()
for t in threads:
t.start()
for t in threads:
t.join()
insp.join()


if __name__ == "__main__":
unittest.main()
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fix a race condition in free-threaded builds where cross-thread frame
inspection via :mod:`pdb` could cause a spurious assertion failure in
:c:func:`PyFrame_GetBack`.
2 changes: 1 addition & 1 deletion Objects/codeobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -3406,7 +3406,7 @@ create_tlbc_lock_held(PyInterpreterState *interp, PyCodeObject *co, Py_ssize_t i
}
copy_code(interp, (_Py_CODEUNIT *) bc, co);
assert(tlbc->entries[idx] == NULL);
tlbc->entries[idx] = bc;
_Py_atomic_store_ptr_release(&tlbc->entries[idx], bc);
return (_Py_CODEUNIT *) bc;
}

Expand Down
9 changes: 7 additions & 2 deletions Objects/frameobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -2399,10 +2399,15 @@ PyFrameObject*
PyFrame_GetBack(PyFrameObject *frame)
{
assert(frame != NULL);
assert(!_PyFrame_IsIncomplete(frame->f_frame));
/* Use an acquire load so that if we observe the new f_frame pointer
* published by take_ownership()'s release store, we also observe
* new_frame->owner == FRAME_OWNED_BY_FRAME_OBJECT. */
_PyInterpreterFrame *f_frame =
(_PyInterpreterFrame *)_Py_atomic_load_ptr_acquire(&frame->f_frame);
assert(!_PyFrame_IsIncomplete(f_frame));
PyFrameObject *back = frame->f_back;
if (back == NULL) {
_PyInterpreterFrame *prev = frame->f_frame->previous;
_PyInterpreterFrame *prev = f_frame->previous;
prev = _PyFrame_GetFirstComplete(prev);
if (prev) {
back = _PyFrame_GetFrameObject(prev);
Expand Down
14 changes: 12 additions & 2 deletions Python/frame.c
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,25 @@ take_ownership(PyFrameObject *f, _PyInterpreterFrame *frame)
// _PyFrame_Copy takes the reference to the executable,
// so we need to restore it.
new_frame->f_executable = PyStackRef_DUP(new_frame->f_executable);
f->f_frame = new_frame;
new_frame->owner = FRAME_OWNED_BY_FRAME_OBJECT;
/* Fix instr_ptr BEFORE setting owner = FRAME_OWNED_BY_FRAME_OBJECT,
* since _PyFrame_IsIncomplete() short-circuits to false for that owner.
* The original owner (FRAME_OWNED_BY_THREAD) is used for the check. */
if (_PyFrame_IsIncomplete(new_frame)) {
// This may be a newly-created generator or coroutine frame. Since it's
// dead anyways, just pretend that the first RESUME ran:
PyCodeObject *code = _PyFrame_GetCode(new_frame);
new_frame->instr_ptr =
_PyFrame_GetBytecode(new_frame) + code->_co_firsttraceable + 1;
}
/* Set owner BEFORE updating f->f_frame so any concurrent reader that

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does the C memory model guarantee this?
Why can't another thread see f->f_frame = new_frame before new_frame->owner = FRAME_OWNED_BY_FRAME_OBJECT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right, plain C stores provide no ordering guarantee. Fixed in the follow-up commit: take_ownership() now uses _Py_atomic_store_ptr_release() to publish f->f_frame, and PyFrame_GetBack() uses _Py_atomic_load_ptr_acquire() to read it. This establishes the required C11 happens-before relationship — any reader that observes the new f_frame pointer is guaranteed to also observe new_frame->owner == FRAME_OWNED_BY_FRAME_OBJECT.

* observes the new f_frame pointer also sees owner = FRAME_OWNED_BY_FRAME_OBJECT,
* causing _PyFrame_IsIncomplete() to short-circuit to false. */
new_frame->owner = FRAME_OWNED_BY_FRAME_OBJECT;
/* Publish f_frame with a release store so that any concurrent reader
* doing an acquire load of f_frame is guaranteed to also observe
* new_frame->owner == FRAME_OWNED_BY_FRAME_OBJECT (written above),
* satisfying the C11 happens-before relationship. */
_Py_atomic_store_ptr_release(&f->f_frame, new_frame);
assert(!_PyFrame_IsIncomplete(new_frame));
assert(f->f_back == NULL);
_PyInterpreterFrame *prev = _PyFrame_GetFirstComplete(frame->previous);
Expand Down
Loading