Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[backport to 3.5] bpo-29438: Fixed use-after-free in key sharing dict #40

Merged
merged 1 commit into from
Feb 13, 2017

Conversation

methane
Copy link
Member

@methane methane commented Feb 12, 2017

No description provided.

@methane methane added the type-bug An unexpected behavior, bug, or error label Feb 12, 2017
@methane methane changed the title bpo-29438: Fixed use-after-free in key sharing dict [backport to 3.5] bpo-29438: Fixed use-after-free in key sharing dict Feb 12, 2017
@@ -3893,20 +3893,18 @@ _PyObjectDict_SetItem(PyTypeObject *tp, PyObject **dictptr,
}
if (value == NULL) {
res = PyDict_DelItem(dict, key);
if (cached != ((PyDictObject *)dict)->ma_keys) {
Copy link
Member

Choose a reason for hiding this comment

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

Why these lines are deleted?

Copy link
Member Author

Choose a reason for hiding this comment

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

In Python 3.5, PyDict_DelItem won't resize dict. So cached == dict->ma_keys in most cases.

One exception is callback called by weakref or __del__ inserts some items to the dict and resize happened.
In this case, cached != CACHED_KEYS(tp), so DK_DECREF(cached) will be "use-after-free".

In such case, the insertion from the callback would update CACHED_KEYS(tp) correctly.
So clearing CACHED_KEYS(tp) doesn't make sense for most case.

Even when the callback inserts items through __dict__ (not regular attribute access), skipping CACHED_KEYS(tp) = NULL
doesn't cause uncontrolled memory growth. And it happens very rarely.
So I think this code is not worth enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants