GeezerVison is showing github.com — gh-154535: Fix crash when a contextvars.Context iterator is shared between threads by gianghungtien · Pull Request #154586 · python/cpython
Conversation
gianghungtien
commented
Jul 24, 2026
•
edited by bedevere-app
Bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
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[], and i_nodes[] (which holds borrowed node pointers) — and hamt_iterator_next() advanced it with plain reads and writes, with no locking:
static hamt_iter_t
hamt_iterator_next(PyHamtIteratorState *iter, PyObject **key, PyObject **val)
{
if (iter->i_level < 0) { /* plain read */
return I_END;
}
PyHamtNode *current = iter->i_nodes[iter->i_level];
if (IS_BITMAP_NODE(current)) { /* Py_TYPE(current) */
...
A single step is not a single write: descending pushes i_pos[next_level], i_nodes[next_level] and then i_level, while ascending does i_level--. Two threads advancing the same iterator interleave those updates, so i_level ends up pointing at an i_nodes[] slot that was never filled in for that level. Because i_nodes holds borrowed pointers, the very next step dereferences a stale or NULL node in IS_BITMAP_NODE(current) (Py_TYPE) or Py_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() — via hamt_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 contextvars sibling 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 whole hamt_iterator_next() → *_bitmap_next / *_array_next / *_collision_next descent, 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_SECTION compiles 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 in Objects/setobject.c.
_PyHamt_Eq() also drives hamt_iterator_next(), but over a stack-local PyHamtIteratorState that is private to the call, so it needs no locking. The header comment on PyHamtIteratorState now records which of the two cases requires the lock.
Concurrent iteration over a shared Context iterator 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 main on 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):
run 1 exit=-1073741819
run 2 exit=-1073741819
run 3 exit=-1073741819
run 4 exit=-1073741819
run 5 exit=-1073741819
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_iterator keeps 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 existing test_iteration / test_shared_iterator cases.
Also run green:
test_free_threading (full package), test_context, test_contextlib, test_asyncio.test_context on the free-threaded build
test_context, test_contextlib, test_asyncio, test_capi on the default x64 build
python_d -m test -R 3:3 test_context test_contextlib — no refleaks
Tools/patchcheck/patchcheck.py — clean
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…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.
|
Seems you've also discovered that the new free-threaded output directories aren't in .gitignore.
You won't get merged until all those files you added are removed, and we should fix the .gitignore in its own PR
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Sorry about that, and thank you for catching it — the build artifacts were swept in by an automated commit on my side, not deliberately. I've removed them: the branch is force-pushed and this PR is back to the four files it should have been (Python/hamt.c, Include/internal/pycore_hamt.h, the test, and the NEWS entry).
And yes — PCbuild/python.props puts free-threaded builds in win32t/amd64t/arm32t/arm64t, but .gitignore only lists the GIL-enabled four, so build.bat --disable-gil leaves its whole output untracked. Fixed separately in #154641, as you suggested.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Overall, this looks good. The added INCREF/DECREFs on key/value are unneeded, I think. Suggested patch below.
--- a/Python/hamt.c
+++ b/Python/hamt.c
@@ -2475,8 +2475,8 @@ static PyObject *
hamt_baseiter_tp_iternext(PyObject *op)
{
PyHamtIterator *it = (PyHamtIterator*)op;
- PyObject *key = NULL;
- PyObject *val = NULL;
+ PyObject *key;
+ PyObject *val;
hamt_iter_t res;
/* The depth-first cursor (i_level, i_pos and i_nodes) is stored in the
@@ -2487,15 +2487,16 @@ hamt_baseiter_tp_iternext(PyObject *op)
dereference a stale or NULL node. Serialize the whole descent on the
iterator; the HAMT itself is immutable, so nothing else needs locking.
+ key and val are borrowed references into the tree and stay valid after
+ the lock is released: they are pinned via hi_obj, which this function
+ never drops, and tp_clear (the only other code that drops it) cannot
+ run concurrently because an in-flight tp_iternext call implies the
+ caller owns a reference to the iterator, keeping it reachable.
+
Concurrent iteration can still skip or repeat items, which matches the
behaviour of the other free-threaded iterators. */
Py_BEGIN_CRITICAL_SECTION(op);
res = hamt_iterator_next(&it->hi_iter, &key, &val);
- if (res == I_ITEM) {
- /* hamt_iterator_next() returns borrowed references. */
- Py_INCREF(key);
- Py_INCREF(val);
- }
Py_END_CRITICAL_SECTION();
switch (res) {
@@ -2504,10 +2505,7 @@ hamt_baseiter_tp_iternext(PyObject *op)
return NULL;
case I_ITEM: {
- PyObject *item = (*(it->hi_yield))(key, val);
- Py_DECREF(key);
- Py_DECREF(val);
- return item;
+ return (*(it->hi_yield))(key, val);
}
default: {
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…rnext()
Apply review feedback from nascheme: the temporary references taken
around the critical section are unnecessary.
* key and val are borrowed into the HAMT, which is immutable and is
pinned for the iterator's lifetime by hi_obj, so they stay valid
after the critical section is released. hamt_baseiter_tp_clear() is
the only code that drops hi_obj and cannot run while a tp_iternext
call is in flight, since the caller owns a reference to the iterator.
* All three yield functions already take their own references
(Py_NewRef() for keys and values, _PyTuple_FromPair() for items), so
the returned object never depended on the temporary ones.
* Record that lifetime argument in the comment above the lock.
This also restores the key and val declarations to the form main
already uses.
|
Thanks — you're right, applied verbatim.
For the record, three things make the borrowed references safe here:
hi_obj is a strong reference to the PyHamtObject, which strong-references h_root, and the HAMT is immutable — so every key and value reachable from the root stays alive for as long as the iterator holds hi_obj. Releasing the critical section can't make them dangle.
hamt_baseiter_tp_clear() is the only code that drops hi_obj, and it runs only from the GC on cyclic garbage or from tp_dealloc at refcount 0. Neither can happen while a tp_iternext call is in flight, since the caller owns a reference to the iterator — as you say.
- The returned object never depended on the temporary references anyway:
hamt_iter_yield_keys() / hamt_iter_yield_values() use Py_NewRef(), and hamt_iter_yield_items() uses _PyTuple_FromPair(), which increfs both.
So the pair only cost two extra atomic refcount updates per step, on objects that are by construction contended across the iterating threads. Your comment is kept as written.
With this the diff against main in hamt_baseiter_tp_iternext() is down to the comment plus the critical section — the declarations and the switch are back to being identical to main.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…h-154535-hamt-iterator-thread-safety
# Conflicts:
# Lib/test/test_free_threading/test_iteration.py
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|