Skip to content

gh-158198: Fix crash when de-inlining dict while freeing owning object - #160114

Open
DinoV wants to merge 2 commits into
python:mainfrom
DinoV:gh158198
Open

DinoV wants to merge 2 commits into
python:mainfrom
DinoV:gh158198

Conversation

@DinoV

@DinoV DinoV commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

When we have an object with a materialized dictionary that's being deleted we have a race condition with converting the dict to no longer use the inline values. While we lock the dict during the conversion we don't lock the dict when we're freeing the object. This can lead to us freeing the object while the dict is trying to read them into the new non-inline values.

We already update ma_values with a release write but we're nulling it out before we've moved the values over. This modifies it so that we are now nulling it out after we've moved the values out. And then we update the object clearing to do an acquire read of the values.

Comment thread Objects/dictobject.c Outdated
FT_ATOMIC_STORE_PTR_RELEASE(oldvalues->values[i], NULL);
Py_XDECREF(tmp);
}
set_values(mp, NULL);

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.

I think we can observe an inconsistent state here (non-NULL values but keys are Py_EMPTY_KEYS) if Py_XDECREF(tmp) invokes a finalizer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right, and it's actually not necessary here, I was jut being over-zealous. This is actually the non-embedded case so the values don't live in the object, but in the embedded case we're just zeroing out the values, and so the racing thread will take the lock.

@bedevere-app

bedevere-app Bot commented Oct 21, 2026

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@mpage mpage left a comment

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.

LGTM!

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.

2 participants