Fix use-after-free race condition in C extension for free-threaded CPython (3.13t+)#1317
Open
rodrigobnogueira wants to merge 6 commits intoaio-libs:masterfrom
Open
Conversation
ff45477 to
0b04769
Compare
0af9e6a to
e1cbca0
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1317 +/- ##
=======================================
Coverage 99.85% 99.85%
=======================================
Files 26 27 +1
Lines 3513 3543 +30
Branches 253 259 +6
=======================================
+ Hits 3508 3538 +30
Misses 3 3
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add per-object locking via Py_BEGIN_CRITICAL_SECTION at all public-facing entry points in the C extension to prevent use-after-free crashes when iterating and mutating a MultiDict concurrently. The root cause is _md_resize() reallocating md->keys without any synchronization, causing concurrent iterators to access freed memory. This fix wraps all public methods in Py_BEGIN_CRITICAL_SECTION(self), matching CPython's own dict locking model. On GIL builds the macros are no-ops (via pythoncapi_compat.h). Adds tests/test_free_threading.py with a concurrent stress test that previously crashed with SIGSEGV and now completes cleanly.
On non-free-threaded builds, Py_END_CRITICAL_SECTION() expands to just '}'. When preceded by a goto label like 'cs_done:', this creates a C23 extension that clang -Werror rejects. Adding an empty statement (;) after the label makes it valid C11.
In debug builds, Py_BEGIN_CRITICAL_SECTION can suspend during Python calls (e.g. __eq__ in md_calc_identity), allowing another thread to enter. The md_replace finder pattern uses hash=-1 markers internally, and ASSERT_CONSISTENT inside md_finder_cleanup catches this temporary inconsistency, calling abort(). Remove del operations from the writer loop since simple set+read is sufficient to exercise the core resize race fix.
62dce51 to
8121e70
Compare
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.
Description
This PR fixes a critical memory-safety race condition (Use-After-Free) that results in a process segmentation fault or abort (stack smashing) when iterating over and modifying a
MultiDictconcurrently in CPython free-threaded mode (3.13t+).Context
When running without the GIL, iterating over
md.keys()/md.items()or view objects could safely yield memory pointers pointing toMultiDictObject->keys. However, if the dictionary resized concurrently using_md_resize(), the keys table was reallocated and immediatelyfree()d, leaving concurrent iterators reading stale heap pointers. This explicitly leaked and corrupted process memory resulting in an ASAN/SEGV error on theREADinside the iterator buffers.Proposed Approach
To safely resolve this while adhering to Python 3.13 free-threaded lock mechanisms, we matched CPython's own
dictconcurrent locking pattern. This involves addingPy_BEGIN_CRITICAL_SECTION(self)/Py_END_CRITICAL_SECTION()at every public-facing entry point in the C extension. Thepythoncapi_compat.hshim already natively skips these macros on pre-3.13 monolithic GIL designs, making this 100% zero-overhead everywhere except 3.13t._multidict.c(__getitem__,__setitem__,__delitem__,add,getall,pop,copy, etc.),iter.h,cs_donefor safety unloads inviews.h.Verification
test_free_threading.py): Added a new threading test module that heavily threadsCIMultiDictadding and deleting inputs while natively reading iterations concurrently.finder->version != finder->md->versionsuccessfully, naturally abandoning the iteration and surfacing standardRuntimeError('MultiDict is changed during iteration')without throwing a C segmentation fault! This matches native single-threaded Py-behavior correctly.clang-formatand confirmed 100% test coverage matches the baseline branch overpython3.13t.