gh-154535: Fix crash when a contextvars.Context iterator is shared between threads#154586
Open
gianghungtien wants to merge 1 commit into
Open
gh-154535: Fix crash when a contextvars.Context iterator is shared between threads#154586gianghungtien wants to merge 1 commit into
gianghungtien wants to merge 1 commit into
Conversation
…red between threads The HAMT iterator that backs iteration over a contextvars.Context keeps its entire depth-first cursor -- i_level, i_pos and the borrowed node pointers in i_nodes -- inside the iterator object, and advanced it with plain reads and writes. Two threads calling next() on the same iterator interleaved those updates, leaving i_level pointing at an i_nodes slot that was never filled in, so the next step dereferenced a stale or NULL node and crashed. Advance the cursor with the iterator locked, so the whole descent is atomic. The HAMT itself is immutable, so no other locking is required. As with the other free-threaded iterators, concurrent iteration may still skip or repeat items, but it no longer crashes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the SIGSEGV reported in #154535.
The bug
Python/hamt.c's iterator keeps its entire depth-first cursor inside the iterator object —i_level,i_pos[], andi_nodes[](which holds borrowed node pointers) — andhamt_iterator_next()advanced it with plain reads and writes, with no locking:A single step is not a single write: descending pushes
i_pos[next_level],i_nodes[next_level]and theni_level, while ascending doesi_level--. Two threads advancing the same iterator interleave those updates, soi_levelends up pointing at ani_nodes[]slot that was never filled in for that level. Becausei_nodesholds borrowed pointers, the very next step dereferences a stale orNULLnode inIS_BITMAP_NODE(current)(Py_TYPE) orPy_SIZE(node)— a wild-pointer read, not merely a torn value. No concurrent structural change is needed; cursor desync alone is enough.This is safe under the GIL, where only one thread runs the advance at a time.
That iterator backs all iteration over a
contextvars.Context—iter(ctx),ctx.keys(),ctx.values(),ctx.items()— viahamt_baseiter_tp_iternext().Per the strategy in #124397, C iterators should get the minimal changes needed so that concurrent use may return duplicate values, skip values, or raise — but must not crash. The HAMT iterator had never been brought under that hardening; it is the
contextvarssibling of the already-fixed dict (#154130) and set (#144357) shared-iterator crashes.The fix
Take a critical section on the iterator in
hamt_baseiter_tp_iternext(), spanning the wholehamt_iterator_next()→*_bitmap_next/*_array_next/*_collision_nextdescent, so no other thread can observe the cursor half-updated. The HAMT itself is immutable, so nothing else needs locking, and the default (GIL-enabled) build is unaffected —Py_BEGIN_CRITICAL_SECTIONcompiles away there.The borrowed key/value are strengthened inside the critical section and the yield function is called after releasing it, so no allocation happens under the lock — matching the
setiter_iternext()idiom inObjects/setobject.c._PyHamt_Eq()also driveshamt_iterator_next(), but over a stack-localPyHamtIteratorStatethat is private to the call, so it needs no locking. The header comment onPyHamtIteratorStatenow records which of the two cases requires the lock.Concurrent iteration over a shared
Contextiterator may still skip or repeat items. That is the documented expectation for free-threaded C iterators, and the added test asserts only that no item is invented — the same contract used by the existing tuple/list/range tests in the file.Verification
Free-threaded build of
mainon Windows x64 (PCbuild\build.bat -p x64 -c Release --disable-gil),PYTHON_GIL=0.The reproducer from the issue, before the fix — 5/5 runs die with
0xC0000005(access violation):After the fix, 5/5 runs complete cleanly.
The new regression test,
ContendedContextIterationTest, reproduces it the same way: 5/5 crashes before the fix ("Windows fatal exception: access violation"), 8/8 passes after.A single pass over a shared iterator turned out to be over too quickly to reliably desync the cursor, so
test_restarted_shared_iteratorkeeps replacing the iterator as it is exhausted to hold the threads on top of each other — that is the variant that reproduces. The class also inherits the existingtest_iteration/test_shared_iteratorcases.Also run green:
test_free_threading(full package),test_context,test_contextlib,test_asyncio.test_contexton the free-threaded buildtest_context,test_contextlib,test_asyncio,test_capion the default x64 buildpython_d -m test -R 3:3 test_context test_contextlib— no refleaksTools/patchcheck/patchcheck.py— cleancontextvars.Contextiterator across threads crashes (HAMT iterator cursor corruption) under free-threading #154535