From a66a46dd20e73cb60a467f854f8df8c735687605 Mon Sep 17 00:00:00 2001 From: Stefan Behnel Date: Sat, 18 Jul 2020 09:43:24 +0200 Subject: [PATCH 1/4] bpo-41295: Reimplement the Carlo Verre "hackcheck": walk down the MRO backwards to find the type that originally defined the final tp_setattro, then make sure we are not jumping over intermediate C-level bases with the Python-level call. --- Lib/test/test_descr.py | 36 ++++++++++++++++++++++++++++++++++++ Objects/typeobject.c | 29 +++++++++++++++++++++-------- 2 files changed, 57 insertions(+), 8 deletions(-) diff --git a/Lib/test/test_descr.py b/Lib/test/test_descr.py index 7bb6f2bb4b30b7..964aea03fcd125 100644 --- a/Lib/test/test_descr.py +++ b/Lib/test/test_descr.py @@ -4308,6 +4308,42 @@ def test_carloverre(self): else: self.fail("Carlo Verre __delattr__ succeeded!") + def test_carloverre_multiinherit_valid(self): + class A(type): + def __setattr__(cls, key, value): + type.__setattr__(cls, key, value) + + class B: + pass + + class C(B, A): + pass + + obj = C('D', (object,), {}) + try: + obj.test = True + except TypeError: + self.fail("setattr through direct base types should be legal") + + def test_carloverre_multiinherit_invalid(self): + class A(type): + def __setattr__(cls, key, value): + object.__setattr__(cls, key, value) # this should fail! + + class B: + pass + + class C(B, A): + pass + + obj = C('D', (object,), {}) + try: + obj.test = True + except TypeError: + pass + else: + self.fail("setattr through indirect base types should be rejected") + def test_weakref_segfault(self): # Testing weakref segfault... # SF 742911 diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 16f95f0e1bf7f9..fa3f1c608197a4 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -5961,25 +5961,38 @@ hackcheck(PyObject *self, setattrofunc func, const char *what) return 1; } assert(PyTuple_Check(mro)); + + /* Find the base type that defined the type's slot function. */ + PyTypeObject *defining_type = type; Py_ssize_t i, n; n = PyTuple_GET_SIZE(mro); - for (i = 0; i < n; i++) { + for (i = n-1; i >= 0; i--) { PyTypeObject *base = (PyTypeObject*) PyTuple_GET_ITEM(mro, i); - if (base->tp_setattro == func) { - /* 'func' is the earliest non-Python implementation in the MRO. */ + if (base->tp_setattro == slot_tp_setattro) { + /* Ignore Python classes: + they never define their own C-level setattro. */ + } + else if (base->tp_setattro == type->tp_setattro) { + /* This (base) type defines the type's final setattr => stop. */ + defining_type = base; break; - } else if (base->tp_setattro != slot_tp_setattro) { - /* 'base' is not a Python class and overrides 'func'. - Its tp_setattro should be called instead. */ + } + } + + /* Reject calls that jump over intermediate overrides. */ + PyTypeObject *base = defining_type; + while (base && base->tp_setattro != func) { + if (base->tp_setattro != slot_tp_setattro) { + /* 'base' is not a Python class and overrides 'func' in the + C-level bases. Its tp_setattro should be called instead. */ PyErr_Format(PyExc_TypeError, "can't apply this %s to %s object", what, type->tp_name); return 0; } + base = base->tp_base; } - /* Either 'func' is not in the mro (which should fail when checking 'self'), - or it's the right slot function to call. */ return 1; } From 7f1b1d998014921e4520dd5b2e18b51dd7f6cec5 Mon Sep 17 00:00:00 2001 From: Stefan Behnel Date: Sat, 18 Jul 2020 14:02:06 +0200 Subject: [PATCH 2/4] bpo-41295: Minor code cleanups. --- Objects/typeobject.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index fa3f1c608197a4..ca2383ad86faa5 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -5962,29 +5962,27 @@ hackcheck(PyObject *self, setattrofunc func, const char *what) } assert(PyTuple_Check(mro)); - /* Find the base type that defined the type's slot function. */ + /* Find the (base) type that defined the type's slot function. */ PyTypeObject *defining_type = type; - Py_ssize_t i, n; - n = PyTuple_GET_SIZE(mro); - for (i = n-1; i >= 0; i--) { + Py_ssize_t i; + for (i = PyTuple_GET_SIZE(mro) - 1; i >= 0; i--) { PyTypeObject *base = (PyTypeObject*) PyTuple_GET_ITEM(mro, i); if (base->tp_setattro == slot_tp_setattro) { /* Ignore Python classes: they never define their own C-level setattro. */ } else if (base->tp_setattro == type->tp_setattro) { - /* This (base) type defines the type's final setattr => stop. */ defining_type = base; break; } } - /* Reject calls that jump over intermediate overrides. */ + /* Reject calls that jump over intermediate C-level overrides. */ PyTypeObject *base = defining_type; while (base && base->tp_setattro != func) { if (base->tp_setattro != slot_tp_setattro) { - /* 'base' is not a Python class and overrides 'func' in the - C-level bases. Its tp_setattro should be called instead. */ + /* 'base' is not a Python class and overrides 'func'. + Its tp_setattro should be called instead. */ PyErr_Format(PyExc_TypeError, "can't apply this %s to %s object", what, From 517d287d9dcd4e3bfb07a5784f80570561d1b0e4 Mon Sep 17 00:00:00 2001 From: Stefan Behnel Date: Sat, 18 Jul 2020 14:27:49 +0200 Subject: [PATCH 3/4] Add news entry --- .../Core and Builtins/2020-07-18-08-15-32.bpo-41295.pu8Ezo.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2020-07-18-08-15-32.bpo-41295.pu8Ezo.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2020-07-18-08-15-32.bpo-41295.pu8Ezo.rst b/Misc/NEWS.d/next/Core and Builtins/2020-07-18-08-15-32.bpo-41295.pu8Ezo.rst new file mode 100644 index 00000000000000..d61fd8f0a29683 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2020-07-18-08-15-32.bpo-41295.pu8Ezo.rst @@ -0,0 +1,3 @@ +Resolve a regression in CPython 3.8.4 where defining "__setattr__" in a +multi-inheritance setup and calling up the hierarchy chain could fail +if builtins/extension types were involved in the base types. From df98894b5daa5236e3831dcd88b7e557332693e7 Mon Sep 17 00:00:00 2001 From: Stefan Behnel Date: Sat, 18 Jul 2020 22:57:10 +0200 Subject: [PATCH 4/4] Implement suggestions from code review. --- Lib/test/test_descr.py | 4 ++-- Objects/typeobject.c | 10 ++++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_descr.py b/Lib/test/test_descr.py index 964aea03fcd125..9738fb52a04bf2 100644 --- a/Lib/test/test_descr.py +++ b/Lib/test/test_descr.py @@ -4308,7 +4308,7 @@ def test_carloverre(self): else: self.fail("Carlo Verre __delattr__ succeeded!") - def test_carloverre_multiinherit_valid(self): + def test_carloverre_multi_inherit_valid(self): class A(type): def __setattr__(cls, key, value): type.__setattr__(cls, key, value) @@ -4325,7 +4325,7 @@ class C(B, A): except TypeError: self.fail("setattr through direct base types should be legal") - def test_carloverre_multiinherit_invalid(self): + def test_carloverre_multi_inherit_invalid(self): class A(type): def __setattr__(cls, key, value): object.__setattr__(cls, key, value) # this should fail! diff --git a/Objects/typeobject.c b/Objects/typeobject.c index ca2383ad86faa5..49b3c859e35c11 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -5978,9 +5978,12 @@ hackcheck(PyObject *self, setattrofunc func, const char *what) } /* Reject calls that jump over intermediate C-level overrides. */ - PyTypeObject *base = defining_type; - while (base && base->tp_setattro != func) { - if (base->tp_setattro != slot_tp_setattro) { + for (PyTypeObject *base = defining_type; base; base = base->tp_base) { + if (base->tp_setattro == func) { + /* 'func' is the right slot function to call. */ + break; + } + else if (base->tp_setattro != slot_tp_setattro) { /* 'base' is not a Python class and overrides 'func'. Its tp_setattro should be called instead. */ PyErr_Format(PyExc_TypeError, @@ -5989,7 +5992,6 @@ hackcheck(PyObject *self, setattrofunc func, const char *what) type->tp_name); return 0; } - base = base->tp_base; } return 1; }