Skip to content

gh-154535: Fix crash when a contextvars.Context iterator is shared between threads#154586

Open
gianghungtien wants to merge 1 commit into
python:mainfrom
gianghungtien:gh-154535-hamt-iterator-thread-safety
Open

gh-154535: Fix crash when a contextvars.Context iterator is shared between threads#154586
gianghungtien wants to merge 1 commit into
python:mainfrom
gianghungtien:gh-154535-hamt-iterator-thread-safety

Conversation

@gianghungtien

@gianghungtien gianghungtien commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

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.Contextiter(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

…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant