Skip to content

gh-160496: Clear inline values with atomic release stores - #160510

Merged
kumaraditya303 merged 2 commits into
python:mainfrom
mpage:gh-160496-dict-race
Oct 26, 2026
Merged

gh-160496: Clear inline values with atomic release stores#160510
kumaraditya303 merged 2 commits into
python:mainfrom
mpage:gh-160496-dict-race

Conversation

@mpage

@mpage mpage commented Oct 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Py_CLEAR writes NULL non-atomically, racing with the acquire load in _PyObject_TryGetInstanceAttribute. Invalidating the inline values does not exclude lookups already in flight.

Authored with AI.

@mpage
pythongh-160496: Clear inline values with atomic release stores
0bdc8b2 
Py_CLEAR writes NULL non-atomically, racing with the acquire load in
_PyObject_TryGetInstanceAttribute. Invalidating the inline values does not
exclude lookups already in flight. Use a release store before decref, preserving
the clear-before-decref ordering.
@bedevere-app bedevere-app Bot mentioned this pull request Oct 23, 2026
Closed
@mpage
mpage requested review from DinoV and nascheme October 23, 2026 21:51
@mpage
mpage marked this pull request as ready for review October 23, 2026 21:52
msullivan
msullivan reviewed Oct 23, 2026
View reviewed changes
Comment thread Objects/dictobject.c
for (Py_ssize_t i = 0; i < values->capacity; i++) {
Py_CLEAR(values->values[i]);
PyObject *value = values->values[i];
FT_ATOMIC_STORE_PTR_RELEASE(values->values[i], NULL);

@msullivan msullivan Oct 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth a comment about what the conflicting reads are?

@nascheme

nascheme commented Oct 23, 2026

Copy link
Copy Markdown
Member

Maybe more explicit about where the load is, e.g.

  // Pair with the acquire load in _PyObject_TryGetInstanceAttribute(),
  // which may read this slot after passing the initial validity check.
  FT_ATOMIC_STORE_PTR_RELEASE(values->values[i], NULL);

Or, do you not want to explicitly name the function?

@mpage

mpage commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Maybe more explicit about where the load is, e.g.

  // Pair with the acquire load in _PyObject_TryGetInstanceAttribute(),
  // which may read this slot after passing the initial validity check.
  FT_ATOMIC_STORE_PTR_RELEASE(values->values[i], NULL);

Or, do you not want to explicitly name the function?

I was explicitly not naming the function to avoid the situation where the comment gets out of date due to refactoring.

@kumaraditya303
kumaraditya303 merged commit a9049ee into python:main Oct 26, 2026
57 checks passed
@miss-islington-app

miss-islington-app Bot commented Oct 26, 2026

Copy link
Copy Markdown

Thanks @mpage for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

@miss-islington-app

miss-islington-app Bot commented Oct 26, 2026

Copy link
Copy Markdown

Thanks @mpage for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.15.
🐍🍒⛏🤖

@bedevere-app

bedevere-app Bot commented Oct 26, 2026

Copy link
Copy Markdown

GH-160709 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Oct 26, 2026
@bedevere-app

bedevere-app Bot commented Oct 26, 2026

Copy link
Copy Markdown

GH-160710 is a backport of this pull request to the 3.15 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.15 bugs and security fixes label Oct 26, 2026
kumaraditya303 pushed a commit that referenced this pull request Oct 26, 2026
@miss-islington @mpage
[3.14] gh-160496: Clear inline values with atomic release stores (GH-…
6841d98 
…160510) (#160709)

gh-160496: Clear inline values with atomic release stores (GH-160510)
(cherry picked from commit a9049ee)

Co-authored-by: mpage <mpage@meta.com>
kumaraditya303 pushed a commit that referenced this pull request Oct 26, 2026
@miss-islington @mpage
[3.15] gh-160496: Clear inline values with atomic release stores (GH-…
9e5726a 
…160510) (#160710)

gh-160496: Clear inline values with atomic release stores (GH-160510)
(cherry picked from commit a9049ee)

Co-authored-by: mpage <mpage@meta.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nascheme nascheme nascheme approved these changes
@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes
@DinoV DinoV Awaiting requested review from DinoV
@methane methane Awaiting requested review from methane methane is a code owner
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner
+1 more reviewer
@msullivan msullivan msullivan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting merge skip news topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mpage @nascheme @msullivan @kumaraditya303