Skip to content

gh-151627: protect OrderedDict iterator creation#151688

Open
pedramkarimii wants to merge 1 commit into
python:mainfrom
pedramkarimii:gh-151627-odict-iterator-race
Open

gh-151627: protect OrderedDict iterator creation#151688
pedramkarimii wants to merge 1 commit into
python:mainfrom
pedramkarimii:gh-151627-odict-iterator-race

Conversation

@pedramkarimii

Copy link
Copy Markdown

Fixes gh-151627.

In free-threaded builds, OrderedDict iterator creation could read the ordered linked-list head/tail, the current node key, the size, and od_state without holding the OrderedDict critical section. A concurrent clear() or update() could mutate or free the linked-list nodes while odictiter_new() was taking that initial iterator snapshot, leading to a data race and a crash/use-after-free.

This change protects the initial iterator snapshot in odictiter_new() with the OrderedDict critical section. The mutation paths already update the linked-list state while holding the OrderedDict lock, so taking the iterator's initial current key, size, state, and object reference under the same critical section avoids racing with concurrent clear()/update().

This is separate from gh-151633/gh-151634, which address Counter.update() in _collectionsmodule.c. This PR targets OrderedDict iterator construction in Objects/odictobject.c.

A free-threading regression test was added that repeatedly creates forward and reversed OrderedDict iterators while another thread concurrently clears and updates the same OrderedDict.

Tests run:

./python -m test test_free_threading.test_collections -v
./python -m test test_free_threading.test_collections -v -m test_iterator_update_clear_race
./python -m test test_ordered_dict -v
./python ../gh151627_reproducer.py

@pedramkarimii

Copy link
Copy Markdown
Author

A bit more context on why this approach was chosen:

The reported race happens while odictiter_new() is taking the iterator's initial snapshot. It reads the ordered linked-list head/tail, the current node's key, the size, and od_state. Those fields belong to the same ordered-list state that clear() and update() can mutate, and clear() can also free the nodes while iterator creation is still reading from them.

The mutation paths already update the OrderedDict linked-list state while holding the OrderedDict critical section. This PR makes iterator creation use the same synchronization boundary when taking its initial snapshot. The change is intentionally small and localized: it does not change iteration semantics, and it does not hold the lock while iterating. It only protects the short section where the iterator captures the first key/state from the OrderedDict.

This is why the fix is in odictiter_new() rather than in the iterator next() path. The reported race is during iterator construction, before the iterator has safely pinned the initial current key.

I also checked the related gh-151633/gh-151634 path. That fix targets Counter.update() in _collectionsmodule.c, while this PR targets OrderedDict iterator construction in Objects/odictobject.c, so this addresses a separate race in a different object implementation.

Locally, the reproducer crashed before this change with a negative refcount/assertion failure, and no longer crashed after the change.

Tests run:

./python -m test test_free_threading.test_collections -v
./python -m test test_free_threading.test_collections -v -m test_iterator_update_clear_race
./python -m test test_ordered_dict -v
./python ../gh151627_reproducer.py

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.

Crash (use-after-free) in odictiter_new when an OrderedDict is concurrently iterated and updated/cleared

1 participant