There is a way to access an underlying mapping in MappingProxyType

[serhiy-storchaka]

BPO	43838
Nosy	@gvanrossum, @rhettinger, @ncoghlan, @serhiy-storchaka, @brandtbucher, @domdfcoding
PRs	bpo-43838: Use a copy to delegate types.MappingProxyType operators #27300

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2021-08-07.07:55:34.904>
created_at = <Date 2021-04-14.07:34:28.852>
labels = ['interpreter-core', 'type-bug', '3.8', '3.9', '3.10']
title = 'There is a way to access an underlying mapping in MappingProxyType'
updated_at = <Date 2021-08-07.08:52:38.535>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2021-08-07.08:52:38.535>
actor = 'ncoghlan'
assignee = 'none'
closed = True
closed_date = <Date 2021-08-07.07:55:34.904>
closer = 'gvanrossum'
components = ['Interpreter Core']
creation = <Date 2021-04-14.07:34:28.852>
creator = 'serhiy.storchaka'
dependencies = []
files = []
hgrepos = []
issue_num = 43838
keywords = ['patch']
message_count = 13.0
messages = ['391039', '397244', '397245', '398023', '398025', '398033', '398045', '398072', '398700', '399022', '399057', '399059', '399171']
nosy_count = 6.0
nosy_names = ['gvanrossum', 'rhettinger', 'ncoghlan', 'serhiy.storchaka', 'brandtbucher', 'domdfcoding']
pr_nums = ['27300']
priority = 'normal'
resolution = 'rejected'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue43838'
versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

[serhiy-storchaka]

The purpose of MappingProxyType is to provide a read-only proxy for mapping. It should not expose the underlying mapping because it would invalidate the purpose of read-only proxy. But there is a way to do this using comparison operator:

from types import MappingProxyType

orig = {1: 2}
proxy = MappingProxyType(orig)

class X:
    def __eq__(self, other):
        other[1] = 3

assert proxy[1] == 2
proxy == X()
assert proxy[1] == 3
assert orig[1] == 3

In particularly it allows to modify __dict__ of builtin types.

[serhiy-storchaka]

Example of modifying a builtin type:

>>> class Sneaky:
...     def __eq__(self, other):
...         other['real'] = 42
... 
>>> int.__dict__ == Sneaky()
>>> (1).real
42

But it can also lead to crash (due to outdated type cache):

>>> class Sneaky:
...     def __eq__(self, other):
...         other['bit_length'] = 42
... 
>>> int.__dict__ == Sneaky()
>>> (1).bit_length
Segmentation fault (core dumped)

[ncoghlan]

I stumbled across this independently in bpo-44596, but missed this existing report due to the specific word I was looking for (encapsulation).

In addition to the comparison operand coercion, there's now another access option through the __ror__ method.

The bug in both cases arises from delegating a method/function implementation to the underlying mapping type in a way that invokes the full operand coercion dance. (PyObject_RichCompare in one case, PyNumber_Or in the other)

Delegating these operations to the underlying mapping does make sense, but it needs to be a lower level delegation that bypasses the operand coercion dance, so the other operand only ever sees the proxy object, not the underlying mapping.

[brandtbucher]

I believe that delegating to the actual underlying mapping without exposing it is a bit of a lost cause, since for some type m we still need these to work:

>>> types.MappingProxyType(m({"a": 0)) | types.MappingProxyType(m({"b": 1}))
m({'a': 0, 'b': 1}) 
>>> types.MappingProxyType(m({"a": 0)) == types.MappingProxyType(m({"a": 0}))
True

(Note that because both sides are proxies, it's impossible for any resolution to happen without m explicitly knowing how to handle them unless both mappings are unwrapped simultaneously.)

Instead, the attached PR delegates to a *copy* of the underlying mapping for these operations instead. I think this is the easiest way to approximate the current behavior while maintaining proper encapsulation.

[gvanrossum]

Maybe we should not fix this then? Copying seems unattractive, and the
reason we proxy in the first place is not absolute safety but to make sure
people don’t accidentally update the dict when they intend to update the
class (for the side effect of updating slots when e.g. __add__ is
added/removed.--
--Guido (mobile)

[serhiy-storchaka]

For __or__ we need to copy the content of both mapping to the resulting mapping in any case. We can implement it as {**self, **other}. We should not use the copy() method because it is not a part of the Mapping interface.

For __eq__, no copying is needed if we just re-implement Mapping.__eq__ (with special cases for few known types for performance).

[ncoghlan]

Delegating operations to the underlying mapping is still OK, all we're wanting to bypass is the operand coercion dances in abstract.c and object.c that may expose the underlying mapping to the *other* operand.

We don't want to implicitly copy though, as the performance hit can be non-trivial for large mappings, even if the algorithmic complexity doesn't change.

For the nb_or slot, it should be correct to retrieve and call __or__ from the underlying mapping when the left operand is a MappingProxy instance, and __ror__ when the right operand is a MappingProxy instance (but the left operand is not).

Rich comparison is messier though, since PyObject_RichCompare handles both the operand coercion dance *and* the mapping of the specific comparison operations to their implementation slots.

I don't see a clean solution to that other than defining a new PyObject_DelegateRichCompare helper function for the mapping proxy to use that provides the mapping from specific operands to their implementation slots *without* doing the coercion dance.

However we resolve it, I think it probably won't be worth the risk of backporting the change (this has been the way it currently is for a long time, and it's a "reduce the risk of error" feature rather than any kind of security boundary).

[brandtbucher]

Delegating operations to the underlying mapping is still OK, all we're wanting to bypass is the operand coercion dances in abstract.c and object.c that may expose the underlying mapping to the *other* operand.

But this won't work if *both* operands are proxies, right? The left wrapped mapping will only ever see itself and the right mappingproxy, and the right operand will only ever see itself and the left mappingproxy. The overwhelmingly common case is where these proxies wrap dicts, and dicts only play nicely with other dicts.

I agree that creating redundant copies isn't optimal; it's just the only way that I see we'd be able to fix this without backward compatibility headaches. The fix is easy to explain to users without needing to get into the nuances of operator dispatch or bloating the code to handle weird edge-cases (trust me, I originally tried coding logic to handle all the different possibilities of subclasses and proxies and such before landing on the copying solution).

With that said, I think that if we decide it's really not worth it to copy here, Serhiy's proposal is probably "good enough". Just return a merged dict for the union operation, and implement mapping equality in a sane, usable way. I just worry that it will break backward-compatibility in subtle ways that are basically impossible to fix (comparing equality of proxied OrderedDicts, for example).

[ncoghlan]

Ouch, you're right - I forgot that dict just returned NotImplemented from comparisons and unions and made it the other operand's problem:

>>> d1 = dict(x=1)
>>> d2 = dict(x=1)
>>> from types import MappingProxyType as proxy
>>> d1.__eq__(proxy(d2))
NotImplemented
>>> d1.__or__(proxy(d2))
NotImplemented

Perhaps we could mitigate the performance hit by skipping the copy when the other side is either a builtin dict (subclasses don't count), or a proxy around a builtin dict?