Skip to content

Commit a3ea63c

Browse files
committed
gh-154535: Fix crash when a contextvars.Context iterator is shared 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.
1 parent eef2349 commit a3ea63c

4 files changed

Lines changed: 77 additions & 4 deletions

File tree

Include/internal/pycore_hamt.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,11 @@ extern PyTypeObject _PyHamtItems_Type;
4646
- i_nodes: an array of seven pointers to tree nodes
4747
- i_level: the current node in i_nodes
4848
- i_pos: an array of positions within nodes in i_nodes.
49+
50+
Because i_nodes holds borrowed references, advancing the state is only
51+
safe while nothing else can observe it half-updated. When the state is
52+
embedded in a PyHamtIterator (as opposed to living on the stack), it must
53+
be advanced with the iterator locked -- see hamt_baseiter_tp_iternext().
4954
*/
5055
typedef struct {
5156
PyHamtNode *i_nodes[_Py_HAMT_MAX_TREE_DEPTH];

Lib/test/test_free_threading/test_iteration.py

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import contextvars
12
import threading
23
import unittest
34
from test import support
@@ -112,6 +113,44 @@ def worker():
112113
self.assert_iterator_results(results, list(seq))
113114

114115

116+
class ContendedContextIterationTest(ContendedTupleIterationTest):
117+
# A contextvars.Context is iterated by the HAMT iterator, which keeps its
118+
# whole depth-first cursor -- including borrowed node pointers -- inside
119+
# the iterator object, so sharing one between threads used to crash the
120+
# interpreter (gh-154535).
121+
def make_testdata(self, n):
122+
variables = [contextvars.ContextVar(f'v{i}') for i in range(n)]
123+
ctx = contextvars.Context()
124+
def populate():
125+
for i, var in enumerate(variables):
126+
var.set(i)
127+
ctx.run(populate)
128+
return ctx
129+
130+
def test_restarted_shared_iterator(self):
131+
"""Test a shared iterator that is restarted when it is exhausted"""
132+
# A single pass over a shared iterator is over too quickly to reliably
133+
# desync its cursor. Keep replacing the iterator once it is exhausted
134+
# so that the threads stay on top of each other for a while.
135+
seq = self.make_testdata(16)
136+
cell = [iter(seq)]
137+
results = []
138+
start = threading.Barrier(NUMTHREADS)
139+
def worker():
140+
items = []
141+
start.wait()
142+
for _ in range(NUMITEMS):
143+
try:
144+
items.append(next(cell[0]))
145+
except StopIteration:
146+
cell[0] = iter(seq)
147+
results.extend(items)
148+
threads = self.run_threads(worker)
149+
for t in threads:
150+
t.join()
151+
self.assert_iterator_results(results, seq)
152+
153+
115154
class ContendedRangeIterationTest(ContendedTupleIterationTest):
116155
def make_testdata(self, n):
117156
return range(n)
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
Fix a crash on the :term:`free-threaded <free threading>` build when a single
2+
iterator over a :class:`contextvars.Context` (as returned by :func:`iter`,
3+
:meth:`~contextvars.Context.keys`, :meth:`~contextvars.Context.values` or
4+
:meth:`~contextvars.Context.items`) is advanced from several threads at once.
5+
The underlying HAMT iterator now advances its traversal cursor while holding
6+
the iterator lock.

Python/hamt.c

Lines changed: 27 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
#include "Python.h"
22
#include "pycore_bitutils.h" // _Py_popcount32()
3+
#include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION()
34
#include "pycore_hamt.h"
45
#include "pycore_initconfig.h" // _PyStatus_OK()
56
#include "pycore_long.h" // _PyLong_Format()
@@ -2474,17 +2475,39 @@ static PyObject *
24742475
hamt_baseiter_tp_iternext(PyObject *op)
24752476
{
24762477
PyHamtIterator *it = (PyHamtIterator*)op;
2477-
PyObject *key;
2478-
PyObject *val;
2479-
hamt_iter_t res = hamt_iterator_next(&it->hi_iter, &key, &val);
2478+
PyObject *key = NULL;
2479+
PyObject *val = NULL;
2480+
hamt_iter_t res;
2481+
2482+
/* The depth-first cursor (i_level, i_pos and i_nodes) is stored in the
2483+
iterator and is updated by every step of the descent, so two threads
2484+
advancing the same iterator would interleave their updates and leave
2485+
i_level pointing at an i_nodes slot that was never filled in. Since
2486+
i_nodes holds borrowed pointers, that desync makes the next step
2487+
dereference a stale or NULL node. Serialize the whole descent on the
2488+
iterator; the HAMT itself is immutable, so nothing else needs locking.
2489+
2490+
Concurrent iteration can still skip or repeat items, which matches the
2491+
behaviour of the other free-threaded iterators. */
2492+
Py_BEGIN_CRITICAL_SECTION(op);
2493+
res = hamt_iterator_next(&it->hi_iter, &key, &val);
2494+
if (res == I_ITEM) {
2495+
/* hamt_iterator_next() returns borrowed references. */
2496+
Py_INCREF(key);
2497+
Py_INCREF(val);
2498+
}
2499+
Py_END_CRITICAL_SECTION();
24802500

24812501
switch (res) {
24822502
case I_END:
24832503
PyErr_SetNone(PyExc_StopIteration);
24842504
return NULL;
24852505

24862506
case I_ITEM: {
2487-
return (*(it->hi_yield))(key, val);
2507+
PyObject *item = (*(it->hi_yield))(key, val);
2508+
Py_DECREF(key);
2509+
Py_DECREF(val);
2510+
return item;
24882511
}
24892512

24902513
default: {

0 commit comments

Comments
 (0)