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

[fatelei]

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

• Issue: Use-after-free in OrderedDict.copy via re-entrant __getitem__ #142734

[picnixz]

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]

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]

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);
        }

[picnixz]

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

[fatelei]

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

test update

[picnixz]

Don't do that. If value is NULL it's because PyObject_GetItem failed. I would prefer propagating that specific exception instead. Only if value was correctly retrieved but the state changed should you raise the RuntimeError.

However, as I don't remember exactly, check whether this is consistent with __eq__ as well. What I expect is:

value = call getitem(...)
if (value == NULL) { ... }
if (self->od_state != state) { ...}
rc = call setitem(...)
if (rc < 0) { ... }
if (self->od_state != state) { ...}

[picnixz]

no need to have a private attribute, it's a test.