python / python/cpython

Race between UserDict.__getitem__ and __delitem__ can result in __missing_ not being called when item is missing.

Open
#156,544 5 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

stdlib topic-free-threading type-bug
Dominant language
Python
Stars
77.2k
Forks
35.9k
PR merge metrics
PR metrics pending

Description

https://github.com/python/cpython/blob/71cb9e8b64c4a89971955d3ce8ff393f283aeace/Lib/collections/__init__.py#L1189

I believe there is a race if __getitem__ sees the key exists in self.data and the GIL is released before getting the item. If another thread then calls __delitem__ which removes the item from self.data, when __getitem__ resumes it will attempt to get and return the item which no longer exists resulting in KeyError being raised out of __getitem__. If the class has __missing__ defined it should be called if the item is missing, but that won't occur in this sequence of events.
This is a theoretical bug report based solely on code inspection prompted by a DPO discussion about a different race the GIL does protect against (https://discuss.python.org/t/pep-805-safe-parallel-python/108670/49).

The only concern is that __missing__ will not be called, not that the item is initially seen then not seen.
I have not done a thorough review yet of the rest of the UserDict methods that do a containment check followed by lookup, but my cursory inspection they look to delegate to __getitem__. I am a bit concerned that get() does not call __missing__ if the initial check does not see the item but will if it sees the item and this bug is fixed as proposed below will result in __missing__ being called. This should be consistent.
https://github.com/python/cpython/pull/17910 seems related and I will look into the details of that fix to ensure this fix is consistent (if I end up working on this issue).

I offer a strawman proposal for fixing this by changing __getitem__ to use a single inspection of self.data with something like:
item = self.data.get(key, MISSING) if item is not MISSING: return item ...
Where MISSING is an internal object guaranteed to not be a key in self.data (adding it if something suitable doesn't already exist).

I volunteer to work on this issue. To point me in the right direction are there existing tests (either in collections or elsewhere) that force GIL timing races I can look at as an example of how to write a test to verify this issue is fixed? The challenge is reliably interleaving the initial containment check, then __delitem__, then the subscript access. I'm not sure if this level of testing is feasible based on previous experience with similar issues...the fix usually invalidates the test steps to interleave things properly by removing the ability for them to be interleaved. Any guidance on this would be very appreciated if my offer to work on this is taken up.

Linked PRs
  • gh-156621
  • gh-157287

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at Lib/collections/init.py around line 1189 and inspect the UserDict lookup and deletion behavior described in the report. Review linked PRs gh-156621 and gh-157287 for the current direction, then check whether existing collections tests cover missing and the relevant interleaving. Done means the reported missing-item behavior is resolved consistently with get() and covered by suitable tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.