gh-152405 Safer mapping proxy comparisons

[da-woods]

Essentially:

• if w is a mapping proxy then unwrap it.
• if w uses the dict comparison operator then use that.
• if v->mapping is a frozendict then forward that (because it's immutable so safe)
• otherwise return NOT_IMPLEMENTED (on the basis that it's likely that w's rich comparison can do it through the standard mapping interface.

---

This is a draft (without tests) because two other people have already proposed possible solutions. I just wanted to propose my C code solution, but my assumption is that (if we want to use it) it'll be stolen and merged with someone else's proposed tests.

• Issue: vars's mapping proxy can expose internal dictionary even for built-in types #152405

[da-woods]

mappingproxy_or would need something similar....

[sobolevn]

This might now work with OrderedDict I think 🤔
Let's wait for tests to report that.

[sobolevn]

You don't need the second PyFrozenDict_CheckExact call:

#define PyAnyDict_CheckExact(ob) \
    (PyDict_CheckExact(ob) || PyFrozenDict_CheckExact(ob))

[da-woods]

Note the second check is on v->mapping rather than w so I think it serves a purpose if w is a (any)dict subclass with an overridden rich-compare. It's possible that it's not worth the effort though

[da-woods]

So ordereddict fails. I can easily add another special-case for ordereddict although it does illustrate the point that this changes the behaviour for anything that has a comparison operator that relies on the exact type of what is passed to it.

While looking at ordereddict, I notice that it isn't comparable with frozendict. That's probably worth fixing (but separately).

[da-woods]

I've pushed an equivalent fix to _or since I think that's the only other operator that can leak in this way.

At this stage I'm happy with:

• This being rejected as the wrong solution.
• Someone else to steal this work and finish it off (by adding tests etc.)
• Me to finish this off (probably by stealing the tests in gh-152405: Fix crash of rich-comparing MappingProxyType objects #152483 and adding a few for _or).

[sobolevn]

I would prefer to keep this PR focused on one specific problem :)

We already have 3 versions, let's no make it even more complicated, please

[da-woods]

Sorry! I promise I'm done changing this for now.

Obviously it can be split off separately if cleaner (but probably not worth it while the approach is still undecided).

[sobolevn]

Both our versions fail this test:

import types
from collections.abc import Mapping

class CustomMapping1(Mapping):
    def __init__(self, data):
        self._data = data

    def __getitem__(self, key):
        return self._data[key]

    def __iter__(self):
        return iter(self._data)

    def __len__(self):
        return len(self._data)

    def __contains__(self, item):
        return item in self._data

class CustomMapping2(CustomMapping1):
    def __eq__(self, other):
        return isinstance(other, CustomMapping1) and self._data == other._data

m = types.MappingProxyType(CustomMapping1({'a': 1}))
assert m == CustomMapping2({'a': 1})

it passes on main :(

[da-woods]

I think that's sort of unavoidable with anything that relies on isinstance because it is an observable behaviour change.

My changed _or has a similar failure for UserDict which is kind of the same thing - just an overly strict type check in its __or__ method. I'll keep my promise and won't try to fix it...

If you took the other approach of "dicts are the only thing we care about leaking because type dicts can cause a crash so make a copy of them only" then I think that test would work.

[da-woods]

If you took the other approach of "dicts are the only thing we care about leaking because type dicts can cause a crash so make a copy of them only" then I think that test would work.

i.e. something like this would fix the crash and I think not change anything else

static PyObject *
mappingproxy_richcompare(PyObject *self, PyObject *w, int op)
{
    mappingproxyobject *v = (mappingproxyobject *)self;
    if (op == Py_EQ || op == Py_NE) {
        mappingproxyobject *v = (mappingproxyobject *)self;
        if (PyObject_TypeCheck(w, &PyDictProxy_Type)) {
            w = ((mappingproxyobject *)w)->mapping;
        }
        if (PyDict_CheckExact(v->mapping) && !mappingproxy_is_known_dict_subclass_richcompare(w)) {
            PyObject *v_mapping_copy = PyDict_Copy(v->mapping);
            if (!v_mapping_copy) {
                 return NULL;
            }
            PyObject *result = PyObject_RichCompare(v_mapping_copy, w, op);
            Py_DECREF(v_mapping_copy);
            return result;
        }
        return PyObject_RichCompare(v->mapping, w, op);
    }
    Py_RETURN_NOTIMPLEMENTED;
}

[sobolevn]

Yeap, this is exactly what I thought of after your comment here:

If you took the other approach of "dicts are the only thing we care about leaking because type dicts can cause a crash so make a copy of them only" then I think that test would work.

I will credit you for this amazing idea! Thanks a lot for your help, it was fun debigging this issue 🤝

I will now close this PR and propose to concentrate on #152483 :)