Skip to content

gh-142734: fix UAF in OrderedDict.copy with concurrent mutations by __getitem__#143059

Open
fatelei wants to merge 9 commits intopython:mainfrom
fatelei:issue-142734
Open

gh-142734: fix UAF in OrderedDict.copy with concurrent mutations by __getitem__#143059
fatelei wants to merge 9 commits intopython:mainfrom
fatelei:issue-142734

Conversation

@fatelei
Copy link
Copy Markdown
Contributor

@fatelei fatelei commented Dec 22, 2025

fix OrderedDict copy heap-use-after-free security issue

@picnixz picnixz changed the title gh-142734: fix OrderedDict copy heap-use-after-free security issue gh-142734: fix UAF in OrderedDict.copy with concurrent mutations by __getitem__ Dec 27, 2025
Copy link
Copy Markdown
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

This implementation is not efficient. I think it's better that we just bail during the copy (that is, raise) if we detect a state change (if possible). We had a similar patch in __eq__.

@bedevere-app
Copy link
Copy Markdown

bedevere-app Bot commented Dec 27, 2025

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@fatelei
Copy link
Copy Markdown
Contributor Author

fatelei commented Dec 28, 2025

This implementation is not efficient. I think it's better that we just bail during the copy (that is, raise) if we detect a state change (if possible). We had a similar patch in __eq__.

update

        PyODictObject *self = _PyODictObject_CAST(od);
        size_t state = self->od_state;
        _ODictNode *cur = _odict_FIRST(od);

        while (cur != NULL) {
            if (self->od_state != state) {
                PyErr_SetString(PyExc_RuntimeError,
                                "OrderedDict mutated during iteration");
                goto fail;
            }
            PyObject *key = Py_NewRef(_odictnode_KEY(cur));
            PyObject *value = PyObject_GetItem((PyObject *)od, key);
            if (value == NULL) {
                Py_DECREF(key);
                goto fail;
            }
            if (self->od_state != state) {
                Py_DECREF(key);
                Py_DECREF(value);
                PyErr_SetString(PyExc_RuntimeError,
                                "OrderedDict mutated during iteration");
                goto fail;
            }
            if (PyObject_SetItem((PyObject *)od_copy, key, value) != 0) {
                Py_DECREF(key);
                Py_DECREF(value);
                goto fail;
            }
            Py_DECREF(key);
            Py_DECREF(value);

            cur = _odictnode_NEXT(cur);
        }

Copy link
Copy Markdown
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Please add tests when the dictionary is shrinked or grows during the iteration (not just cleared)

Comment thread Objects/odictobject.c Outdated
Comment thread Objects/odictobject.c Outdated
@fatelei
Copy link
Copy Markdown
Contributor Author

fatelei commented Dec 28, 2025

Please add tests when the dictionary is shrinked or grows during the iteration (not just cleared)

test update

Comment thread Lib/test/test_ordered_dict.py Outdated
Comment thread Misc/NEWS.d/next/Library/2025-12-22-15-02-16.gh-issue-142734.IxVAQh.rst Outdated
Comment thread Objects/odictobject.c Outdated
Comment thread Objects/odictobject.c
Comment thread Lib/test/test_ordered_dict.py Outdated
Comment thread Lib/test/test_ordered_dict.py Outdated
Comment thread Lib/test/test_ordered_dict.py Outdated
Comment thread Lib/test/test_ordered_dict.py Outdated
Comment thread Lib/test/test_ordered_dict.py Outdated
Comment thread Objects/odictobject.c
Comment thread Objects/odictobject.c Outdated
Comment thread Lib/test/test_ordered_dict.py Outdated
Comment thread Lib/test/test_ordered_dict.py Outdated
Comment thread Objects/odictobject.c
Comment thread Lib/test/test_ordered_dict.py Outdated
Copy link
Copy Markdown
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Can you also check that the error message is the correct one:

msg = "OrderedDict mutated during iteration"
self.assertRaisesRegex(RuntimeError, msg, od.copy)

Comment thread Lib/test/test_ordered_dict.py
Comment thread Lib/test/test_ordered_dict.py Outdated
Comment thread Lib/test/test_ordered_dict.py
Comment thread Lib/test/test_ordered_dict.py Outdated
Comment thread Lib/test/test_ordered_dict.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.

2 participants