gh-153176: Fix crash in _PyMem_mi_page_reclaimed on free-threaded debug builds#153307
gh-153176: Fix crash in _PyMem_mi_page_reclaimed on free-threaded debug builds#153307zangjiucheng wants to merge 2 commits into
Conversation
|
I also think we can just relax this assertion. Thanks for your fix! |
| assert(page->qsbr_node.next == NULL); | ||
| // We may be reclaiming a page belonging to a different thread | ||
| // during a stop-the-world event. Find the _PyThreadStateImpl for | ||
| // the page. |
There was a problem hiding this comment.
I'd like to have an assertion for the stw pause.
There was a problem hiding this comment.
I believe that garbage collection may or may not occur in an STW state… Therefore, the main idea here is to relax the restrictions to ensure that the garbage collection process is not blocked.
I verified under lldb that the foreign-page reclaim happens with world_stopped=false everywhere (it's a PyThreadState_Clear teardown path, not GC), so such an assert would re-crash the exact repro; leave the fix as-is (the sibling maybe_free also has no assert), or at most assert the one true invariant tstate->base.interp == _PyThreadState_GET()->interp.
Pls correct me if I missed something :)
There was a problem hiding this comment.
I'm a bit confused as to what's going on, but this seems broken in whatever is calling PyThreadState_Clear.
PyThreadState_Clear should only be called on the current active thread state and thread states are not allowed to migrate between OS threads.
There was a problem hiding this comment.
@colesbury Thanks so much you can take time to review this :)
You're right that the caller clears a non-current tstate. I traced it with lldb on main (free-threaded debug build):
_PyMem_mi_page_reclaimed obmalloc.c:215 <- assert
_PyThreadState_ClearMimallocHeaps pystate.c:3318
PyThreadState_Clear(t1) pystate.c:1902
destroy_interpreter(basic=True) _testinternalcapi.c:2315
This is the helper's "weird out-of-order thread states" path, not a real teardown: it does PyThreadState_Swap(t2) and then PyThreadState_Clear(t1), so t1 is not current. No OS-thread migration is involved: pthread_self() == t1->thread_id == t2->thread_id, both are thread states of the same subinterpreter, and world_stopped == false at the assert. The normal _PyXI_EndInterpreter path does not reproduce.
I relaxed the assertion because PyThreadState_Clear itself only asserts the same interpreter
current_fast_get()->interp == tstate->interp, and core code clears non-current tstates too: interpreter_clear clears the current one last and _PyThreadState_DeleteList walks a list.
How would you prefer this fixed? (Also pls correct me if I missed something..., thx)
…ent thread state _testinternalcapi.destroy_interpreter(basic=True) called PyThreadState_Clear() and PyThreadState_Delete() on t1 while t2 was the current thread state. On a free-threaded debug build this reclaimed mimalloc pages into a heap not owned by the current thread, tripping the assertion in _PyMem_mi_page_reclaimed(). Leave t1 for Py_EndInterpreter() to clean up. Add a regression test. Co-authored-by: Kumar Aditya <kumaraditya@python.org> Co-authored-by: Xiaowei Lu <weixlu420302@gmail.com>
Summary
Fixes #153176.
On free-threaded debug builds, destroying a subinterpreter could abort the
process with:
_PyMem_mi_page_reclaimed()asserted that the page being reclaimed alwaysbelongs to the current thread state. That assumption doesn't hold during
stop-the-world events (e.g. subinterpreter teardown), where a page can be
reclaimed on behalf of a different thread state than the one currently
running. The function already looks up the correct owning thread state via
tstate_from_heap(mi_page_heap(page))— it just also asserted, incorrectly,that this always equals the current thread state. This mirrors the reasoning
in
_PyMem_mi_page_maybe_free(), which does not make the same assumption.This removes the overly-restrictive assertion and adds a regression test
that reproduces the crash via
_testinternalcapi.create_interpreter()/destroy_interpreter(), run in a subprocess so a regression fails the testinstead of aborting the whole test runner.
Test plan
./python -m test test_free_threading.test_interpreters -vmainprior to the fix; confirmed it nolonger reproduces with this change on a free-threaded debug build.