Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions Lib/test/test_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -1180,6 +1180,28 @@ def test_instantiation(self):
class MappingProxyTests(unittest.TestCase):
mappingproxy = types.MappingProxyType

class MappingWithoutCopy(collections.abc.Mapping):
def __init__(self, mapping):
self.mapping = mapping

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

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

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

class RecordingOperand:
def __eq__(self, other):
self.other = other
return None

def __ror__(self, other):
self.other = other
return None

def test_constructor(self):
class userdict(dict):
pass
Expand Down Expand Up @@ -1381,6 +1403,70 @@ def test_union(self):
self.assertDictEqual(mapping, {'a': 0, 'b': 1, 'c': 2})
self.assertDictEqual(other, {'c': 3, 'p': 0})

def test_richcompare_does_not_expose_mapping(self):
mapping = {}
view = self.mappingproxy(mapping)

class Sneaky:
def __eq__(self, other):
other['x'] = 42
return None

self.assertIsNone(view == Sneaky())
self.assertEqual(mapping, {})

other = self.RecordingOperand()
view = self.mappingproxy(self.MappingWithoutCopy(mapping))
self.assertIsNone(view == other)
self.assertIs(type(other.other), self.mappingproxy)

def test_union_does_not_expose_mapping(self):
mapping = {}
view = self.mappingproxy(mapping)
mappingproxy = self.mappingproxy

class SneakyRor:
def __ror__(self, other):
other['x'] = 42
return None

class SneakyOr:
def __or__(self, other):
if type(other) is mappingproxy:
return NotImplemented
other['y'] = 42
return None

self.assertIsNone(view | SneakyRor())
self.assertEqual(mapping, {})
self.assertIsNone(SneakyOr() | view)
self.assertEqual(mapping, {})

other = self.RecordingOperand()
view = self.mappingproxy(self.MappingWithoutCopy(mapping))
self.assertIsNone(view | other)
self.assertIs(type(other.other), self.mappingproxy)

def test_operator_dispatch_does_not_expose_type_dict(self):
code = textwrap.dedent("""
class SneakyEq:
def __eq__(self, other):
other['__mappingproxy_test_eq__'] = lambda self: None
return None

class SneakyRor:
def __ror__(self, other):
other['__mappingproxy_test_or__'] = lambda self: None
return None

vars(list) == SneakyEq()
vars(dict) | SneakyRor()

assert not hasattr(list, '__mappingproxy_test_eq__')
assert not hasattr(dict, '__mappingproxy_test_or__')
""")
assert_python_ok("-c", code)

def test_hash(self):
class HashableDict(dict):
def __hash__(self):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Prevent ``mappingproxy`` operator dispatch from exposing the underlying mapping
during rich comparison and union operations. This fixes a case where reflected
operator methods could access and mutate internal dictionaries, including those
of built-in types.
52 changes: 46 additions & 6 deletions Objects/descrobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -1061,16 +1061,50 @@ static PyMappingMethods mappingproxy_as_mapping = {
0, /* mp_ass_subscript */
};

/* Use a copy of proxied mappings for operator dispatch, so reflected
methods cannot access the underlying mapping. */
static PyObject *
mappingproxy_copy_mapping(PyObject *mapping)
{
PyObject *copy_method;
int res = PyObject_GetOptionalAttr(mapping, &_Py_ID(copy), &copy_method);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think that it is correct: see my proposed alternative #152483

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review @sobolevn and for putting together the alternative approach.

I agree that relying on .copy() for arbitrary mappings is not ideal since it is not part of the Mapping interface. I’ll leave my PR as-is for now and follow the discussion on #152483.

If you would prefer, I can close this PR in favor of #152483, or help with tests/review there.

if (res < 0) {
return NULL;
}
if (res == 0) {
Py_RETURN_NOTIMPLEMENTED;
}

PyObject *copy = PyObject_CallNoArgs(copy_method);
Py_DECREF(copy_method);
return copy;
}

static PyObject *
mappingproxy_as_operand(PyObject *operand)
{
if (PyObject_TypeCheck(operand, &PyDictProxy_Type)) {
return mappingproxy_copy_mapping(((mappingproxyobject *)operand)->mapping);
}
return Py_NewRef(operand);
}

static PyObject *
mappingproxy_or(PyObject *left, PyObject *right)
{
if (PyObject_TypeCheck(left, &PyDictProxy_Type)) {
left = ((mappingproxyobject*)left)->mapping;
left = mappingproxy_as_operand(left);
if (left == NULL || left == Py_NotImplemented) {
return left;
}
if (PyObject_TypeCheck(right, &PyDictProxy_Type)) {
right = ((mappingproxyobject*)right)->mapping;
right = mappingproxy_as_operand(right);
if (right == NULL || right == Py_NotImplemented) {
Py_DECREF(left);
return right;
}
return PyNumber_Or(left, right);
PyObject *result = PyNumber_Or(left, right);
Py_DECREF(left);
Py_DECREF(right);
return result;
}

static PyObject *
Expand Down Expand Up @@ -1234,7 +1268,13 @@ mappingproxy_richcompare(PyObject *self, PyObject *w, int op)
{
mappingproxyobject *v = (mappingproxyobject *)self;
if (op == Py_EQ || op == Py_NE) {
return PyObject_RichCompare(v->mapping, w, op);
PyObject *mapping = mappingproxy_copy_mapping(v->mapping);
if (mapping == NULL || mapping == Py_NotImplemented) {
return mapping;
}
PyObject *result = PyObject_RichCompare(mapping, w, op);
Py_DECREF(mapping);
return result;
}
Py_RETURN_NOTIMPLEMENTED;
}
Expand Down
Loading