Fix the itertools.tee leak, the contextvars and generator races, and _imp's frozen data - #8518
Fix the itertools.tee leak, the contextvars and generator races, and _imp's frozen data#8518youknowone wants to merge 5 commits into
Conversation
The data argument went through deserialize_code(), which reads a code body without the type byte in front of it, so nothing marshal.dumps() produces was accepted. Read it with marshal.loads() and require a code object back. Assisted-by: Claude
The arity check was done by hand on a FuncArgs. Take the arguments through a FromArgs struct instead, which makes withdata keyword-only, and fill in the data it asks for: the frozen encoding is not marshal, so the code is re-serialized into what get_frozen_object() reads back. Assisted-by: Claude
The buffer was a PyRc<PyItertoolsTeeData>, which is not a Python object, so the collector could not walk into it and any cycle running through a tee was uncollectable. Make it the _tee_dataobject type with a traverse, held by PyRef, and split the rest along the same lines: tee() is a function, _tee is the iterator type it builds, and _tee takes a single iterable rather than returning a tuple from __new__. _tee is weak-referenceable, tee() rejects a negative n with ValueError and reserves its tuple fallibly. test_itertools.test_tee passes now; its expectedFailure marker is removed. Assisted-by: Claude
The variable map was a RefCell, the enter flag, the context index, the token's used flag and the variable hash were Cells, and each carried an unsafe impl Sync. A Context or ContextVar shared between threads overlapped their borrows and panicked. The map and the per-variable cache are now PyMutex, the flags and the index are atomics, entering a context is a compare_exchange, and the three unsafe impl Sync are gone. The cache also stopped being read through AtomicCell::as_ptr, which raced a concurrent store on a value holding a PyObjectRef. Values displaced from the map or the cache are dropped after the lock is released: __del__ can come straight back into the same context, and the locks are not reentrant. Assisted-by: Claude
send(), send_none(), throw() and close() read `closed` and `frame.lasti()` before `running` was compare_exchanged, so the frame they went on to resume could be one another thread had already advanced. A resume that decided from `lasti() == 0` that the generator had not started pushes no value onto the value stack, and the code after the yield pops one, which underflows the stack. The compare_exchange now hands back a guard, taken before those reads and released after maybe_close(), so the generator is retired while it is still claimed. Assisted-by: Claude
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] test: cpython/Lib/test/test_itertools.py (TODO: 3) dependencies: dependent tests: (56 tests)
Legend:
|
Follow-up to #8514, from the same fuzzing + static-review catalogs: the two thread-safety defects that #8514 left open, one leak found beside them, and the
_impfollow-ups from that PR's review. One commit per defect._impfrozen data_imp.get_frozen_object(name, data)decodeddatawithmarshal::deserialize_code, which reads a bare code body, while the valueimportlibpasses is a whole marshal value — type byte and all. Every explicitdataargument therefore came back asImportError: Frozen object named '…' is invalid. It is read withmarshal.loadsnow; a non-buffer argument still reportsTypeError.find_frozenthen implementswithdata=True, which returnedNonebefore. The data it hands out is whatget_frozen_objecttakes back, so the stored frozen module — which has its own encoding, not marshal — is re-serialized withmarshal.dumpsinto a memoryview. Its arguments are bound by a#[derive(FromArgs)]struct instead of hand-checkedFuncArgs.itertools.teeleaked its bufferThe buffer shared by the iterators of one
tee()was aPyRc<PyItertoolsTeeData>— a plain refcount the collector cannot walk — so any cycle through a tee iterator was unreachable togcand never freed:The buffer is now the
_tee_dataobjectPython type with a traverse, andtee/_teesplit the way they do in CPython:tee()copies an iterator that can copy itself and only wraps one that cannot.test_itertools.test_teeloses itsexpectedFailure.contextvars is shared between threads (RUSTPY-0019)
A
Context's map and eachContextVar's cache are reachable from every thread that touches them, but were held inRefCell/Cells, with threeunsafe impl Synccovering the hole. Concurrentset/reset/copy_contextpanicked onalready borrowed: BorrowMutError, andContextVar::getread the cache throughAtomicCell::as_ptrwhile another thread wrote it.The map and the cache now sit behind
PyMutex,enteredis acompare_exchangeso two threads cannot both enter oneContext,hash/usedare atomics, and the threeunsafe impl Syncare gone. A value displaced byset/reset/deleteis dropped after the lock is released, so a__del__that calls back into the sameContextcannot deadlock.Generator resume race (RUSTPY-0023)
send(),send_none(),throw()andclose()readclosedandframe.lasti()beforerunningwas compare_exchanged, so the frame they went on to resume could be one another thread had already advanced. A resume that concluded fromlasti() == 0that the generator had not started pushes no value onto the value stack, and thePOP_TOPafter the yield pops one —tried to pop from empty stack, the fatal inExecutingFrame::run.The compare_exchange hands back a guard now, taken before those reads and released after
maybe_close(), so the generator is also retired while it is still claimed and a waiting thread cannot resume a frame that has finished.Tests
New snippets:
stdlib_threading_generator.py(four threads resuming the same generator through a barrier; every yielded value has to reach exactly one caller),stdlib_threading_contextvars.py, and ateecase instdlib_gc.py.test_asyncio,test_threading,test_queue,test_selectors,test_weakref,test_gc,test_context,test_generators,test_coroutines,test_asyncgen,test_yield_from,test_contextlib,test_itertools,test_marshal,test_importliball pass; the CI clippy line is clean.Still open
RUSTPY-0007face 7c — the object-core segfault reported in theselectorsandasyncio_queuesvehicles — is not fixed here. The report has no minimal reproducer for it and marks it as needing a per-crash-dir gdb pass to separate it from the recursion face 7a, whose guards landed in #8514. Driving both vehicle surfaces (selectorswith lying/raising/out-of-rangefileno()from several threads,asyncioqueues plus the_asynciotask registry with non-task objects), eighteen deep-nesting shapes and ten object-core abuse shapes (resurrection in__del__, weakref callbacks,__class__/__bases__reassignment, suspendedframe.clear()) produced no crash on this branch.🤖 Generated with Claude Code