From 72c6e63af45f0fbb7703412c8bde20feabf282d7 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 22 Jul 2021 17:43:10 -0700 Subject: [PATCH 1/5] Add regression tests --- Lib/test/test_types.py | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/Lib/test/test_types.py b/Lib/test/test_types.py index 99b011e3786654c..f5f6f89feec451c 100644 --- a/Lib/test/test_types.py +++ b/Lib/test/test_types.py @@ -1131,6 +1131,47 @@ def test_union(self): self.assertDictEqual(mapping, {'a': 0, 'b': 1, 'c': 2}) self.assertDictEqual(other, {'c': 3, 'p': 0}) + def test_delegated_operators(self): + class MyDict(dict): + def __eq__(self, other): + return super().__eq__(other) + def __or__(self, other): + return super().__or__(other) + def __ror__(self, other): + return super().__ror__(other) + dict_a = {0: "a"} + dict_b = {0: "b"} + mydict_a = MyDict(dict_a) + mydict_b = MyDict(dict_b) + view_dict_a = self.mappingproxy(dict_a) + view_mydict_a = self.mappingproxy(mydict_a) + view_view_dict_a = self.mappingproxy(view_dict_a) + a_mappings = (dict_a, mydict_a) + b_mappings = (dict_b, mydict_b) + a_views = (view_dict_a, view_mydict_a, view_view_dict_a) + for view in a_views: + for mapping in a_mappings + a_views: + self.assertDictEqual(view | mapping, dict_a) + self.assertDictEqual(mapping | view, dict_a) + self.assertEqual(view, mapping) + self.assertEqual(mapping, view) + for mapping in b_mappings: + self.assertDictEqual(view | mapping, dict_b) + self.assertDictEqual(mapping | view, dict_a) + self.assertNotEqual(view, mapping) + self.assertNotEqual(mapping, view) + + def test_bpo_43838(self): + mapping = {} + proxy = self.mappingproxy(mapping) + class Sneaky: + def __eq__(self, other): + other['x'] = 42 + return None + self.assertIs(proxy == Sneaky(), None) + self.assertDictEqual(mapping, {}) + self.assertEqual(proxy, {}) + class ClassCreationTests(unittest.TestCase): From 1c280359ef7e4997a0a2ad805bc87b74e3c3b3fc Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 22 Jul 2021 17:44:15 -0700 Subject: [PATCH 2/5] Delegate mappingproxy ops to copies, not originals --- Objects/descrobject.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/Objects/descrobject.c b/Objects/descrobject.c index 0565992bdb79f7e..b1b1637667cc05b 100644 --- a/Objects/descrobject.c +++ b/Objects/descrobject.c @@ -1032,11 +1032,24 @@ static PyMappingMethods mappingproxy_as_mapping = { static PyObject * mappingproxy_or(PyObject *left, PyObject *right) { + // bpo-43838: We can't just return PyNumber_Or(left->mapping, right) or + // PyNumber_Or(left, right->mapping) here, because the operator dispatch dance can + // expose our hidden mapping to arbitrary code. Perform the operation with a copy of + // the mapping instead: + _Py_IDENTIFIER(copy); if (PyObject_TypeCheck(left, &PyDictProxy_Type)) { left = ((mappingproxyobject*)left)->mapping; + left = _PyObject_CallMethodIdNoArgs(left, &PyId_copy); + if (left == NULL) { + return NULL; + } } if (PyObject_TypeCheck(right, &PyDictProxy_Type)) { right = ((mappingproxyobject*)right)->mapping; + right = _PyObject_CallMethodIdNoArgs(right, &PyId_copy); + if (right == NULL) { + return NULL; + } } return PyNumber_Or(left, right); } @@ -1188,7 +1201,15 @@ mappingproxy_traverse(PyObject *self, visitproc visit, void *arg) static PyObject * mappingproxy_richcompare(mappingproxyobject *v, PyObject *w, int op) { - return PyObject_RichCompare(v->mapping, w, op); + // bpo-43838: We can't just return PyObject_RichCompare(v->mapping, w, op) here, + // because the operator dispatch dance can expose our hidden mapping to arbitrary + // code. Perform the operation with a copy of the mapping instead: + _Py_IDENTIFIER(copy); + PyObject *copy = _PyObject_CallMethodIdNoArgs(v->mapping, &PyId_copy); + if (copy == NULL) { + return NULL; + } + return PyObject_RichCompare(copy, w, op); } static int From 17354534aa15d87298e2763ba33fedab246269f6 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 22 Jul 2021 17:45:04 -0700 Subject: [PATCH 3/5] Update docs --- Doc/library/types.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Doc/library/types.rst b/Doc/library/types.rst index 2dfc0f2fcadf371..9cb0dbc86a60cb1 100644 --- a/Doc/library/types.rst +++ b/Doc/library/types.rst @@ -369,6 +369,12 @@ Standard names are defined for the following types: Updated to support the new union (``|``) operator from :pep:`584`, which simply delegates to the underlying mapping. + .. versionchanged:: 3.10 + + To avoid exposing the actual proxied object to arbitrary code, union and + rich comparison operations now delegate to a copy of the underlying + mapping instead. + .. describe:: key in proxy Return ``True`` if the underlying mapping has a key *key*, else From 6f8fdafdcfdb6517a70dc75d83444cf65666423e Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 22 Jul 2021 17:45:14 -0700 Subject: [PATCH 4/5] blurb add --- .../Core and Builtins/2021-07-22-16-10-14.bpo-43838.a5j8Q3.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2021-07-22-16-10-14.bpo-43838.a5j8Q3.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2021-07-22-16-10-14.bpo-43838.a5j8Q3.rst b/Misc/NEWS.d/next/Core and Builtins/2021-07-22-16-10-14.bpo-43838.a5j8Q3.rst new file mode 100644 index 000000000000000..220f5d66e4117d6 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2021-07-22-16-10-14.bpo-43838.a5j8Q3.rst @@ -0,0 +1,3 @@ +To avoid exposing the actual proxied object to arbitrary code, union and +rich comparison operations on :class:`types.MappingProxyType` now delegate +to a copy of the underlying mapping instead. From 8f9dc09f5bddf98f2aba0076dd6008b480d309b1 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 22 Jul 2021 18:06:46 -0700 Subject: [PATCH 5/5] Fix refleaks --- Objects/descrobject.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/Objects/descrobject.c b/Objects/descrobject.c index b1b1637667cc05b..8c1ad42bf9e4e8f 100644 --- a/Objects/descrobject.c +++ b/Objects/descrobject.c @@ -1044,14 +1044,24 @@ mappingproxy_or(PyObject *left, PyObject *right) return NULL; } } + else { + Py_INCREF(left); + } if (PyObject_TypeCheck(right, &PyDictProxy_Type)) { right = ((mappingproxyobject*)right)->mapping; right = _PyObject_CallMethodIdNoArgs(right, &PyId_copy); if (right == NULL) { + Py_DECREF(left); return NULL; } } - return PyNumber_Or(left, right); + else { + Py_INCREF(right); + } + PyObject *result = PyNumber_Or(left, right); + Py_DECREF(left); + Py_DECREF(right); + return result; } static PyObject * @@ -1209,7 +1219,9 @@ mappingproxy_richcompare(mappingproxyobject *v, PyObject *w, int op) if (copy == NULL) { return NULL; } - return PyObject_RichCompare(copy, w, op); + PyObject *result = PyObject_RichCompare(copy, w, op); + Py_DECREF(copy); + return result; } static int