From 61e35e18a6718844b46ffcf1111a28a93c8d3f9f Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Fri, 21 Jun 2024 16:53:16 -0400 Subject: [PATCH 1/3] gh-120860: Fix a few bugs in `type_setattro` error paths. Moves the logic to update the type's dictionary to its own function in order to make the lock scoping more clear. Also, ensure that `name` is decref'd on the error path. --- Objects/typeobject.c | 78 +++++++++++++++++++++++--------------------- 1 file changed, 41 insertions(+), 37 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 53c40885fd621a..6d1fe7debd7f19 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -5656,6 +5656,42 @@ _Py_type_getattro(PyObject *type, PyObject *name) return _Py_type_getattro_impl((PyTypeObject *)type, name, NULL); } +static int +type_update_dict(PyTypeObject *type, PyDictObject *dict, PyObject *name, + PyObject **old_value) +{ + // We don't want any re-entrancy between when we update the dict + // and call type_modified_unlocked, including running the destructor + // of the current value as it can observe the cache in an inconsistent + // state. Because we have an exact unicode and our dict has exact + // unicodes we know that this will all complete without releasing + // the locks. + if (_PyDict_GetItemRef_Unicode_LockHeld(dict, name, &old_value) < 0) { + return -1; + } + + /* Clear the VALID_VERSION flag of 'type' and all its + subclasses. This could possibly be unified with the + update_subclasses() recursion in update_slot(), but carefully: + they each have their own conditions on which to stop + recursing into subclasses. */ + type_modified_unlocked(type); + + if (_PyDict_SetItem_LockHeld(dict, name, value) < 0) { + PyErr_Format(PyExc_AttributeError, + "type object '%.50s' has no attribute '%U'", + ((PyTypeObject*)type)->tp_name, name); + _PyObject_SetAttributeErrorContext((PyObject *)type, name); + return -1; + } + + if (is_dunder_name(name)) { + return update_slot(type, name); + } + + return 0; +} + static int type_setattro(PyObject *self, PyObject *name, PyObject *value) { @@ -5698,12 +5734,11 @@ type_setattro(PyObject *self, PyObject *name, PyObject *value) assert(!_PyType_HasFeature(metatype, Py_TPFLAGS_INLINE_VALUES)); assert(!_PyType_HasFeature(metatype, Py_TPFLAGS_MANAGED_DICT)); - PyObject *old_value; + PyObject *old_value = NULL; PyObject *descr = _PyType_LookupRef(metatype, name); if (descr != NULL) { descrsetfunc f = Py_TYPE(descr)->tp_descr_set; if (f != NULL) { - old_value = NULL; res = f(descr, (PyObject *)type, value); goto done; } @@ -5719,47 +5754,16 @@ type_setattro(PyObject *self, PyObject *name, PyObject *value) } END_TYPE_LOCK(); if (dict == NULL) { - return -1; + res = -1; + goto done; } } - // We don't want any re-entrancy between when we update the dict - // and call type_modified_unlocked, including running the destructor - // of the current value as it can observe the cache in an inconsistent - // state. Because we have an exact unicode and our dict has exact - // unicodes we know that this will all complete without releasing - // the locks. BEGIN_TYPE_DICT_LOCK(dict); - - if (_PyDict_GetItemRef_Unicode_LockHeld((PyDictObject *)dict, name, &old_value) < 0) { - return -1; - } - - /* Clear the VALID_VERSION flag of 'type' and all its - subclasses. This could possibly be unified with the - update_subclasses() recursion in update_slot(), but carefully: - they each have their own conditions on which to stop - recursing into subclasses. */ - type_modified_unlocked(type); - - res = _PyDict_SetItem_LockHeld((PyDictObject *)dict, name, value); - - if (res == 0) { - if (is_dunder_name(name)) { - res = update_slot(type, name); - } - } - else if (PyErr_ExceptionMatches(PyExc_KeyError)) { - PyErr_Format(PyExc_AttributeError, - "type object '%.50s' has no attribute '%U'", - ((PyTypeObject*)type)->tp_name, name); - - _PyObject_SetAttributeErrorContext((PyObject *)type, name); - } - + res = type_update_dict(type, (PyDictObject *)dict, name, &old_value); assert(_PyType_CheckConsistency(type)); - END_TYPE_DICT_LOCK(); + done: Py_DECREF(name); Py_XDECREF(descr); From 245641017f0f64f03a7f211ef9e9d582ce44bb42 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Fri, 21 Jun 2024 17:21:26 -0400 Subject: [PATCH 2/3] Fix reference to old_value --- Objects/typeobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 6d1fe7debd7f19..05454b9292e22e 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -5666,7 +5666,7 @@ type_update_dict(PyTypeObject *type, PyDictObject *dict, PyObject *name, // state. Because we have an exact unicode and our dict has exact // unicodes we know that this will all complete without releasing // the locks. - if (_PyDict_GetItemRef_Unicode_LockHeld(dict, name, &old_value) < 0) { + if (_PyDict_GetItemRef_Unicode_LockHeld(dict, name, old_value) < 0) { return -1; } From f8d73edb46644aeff4ae408875e808d93e17f9b5 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Fri, 21 Jun 2024 17:47:49 -0400 Subject: [PATCH 3/3] Add missing parameter --- Objects/typeobject.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 05454b9292e22e..71fcb6559fff73 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -5658,7 +5658,7 @@ _Py_type_getattro(PyObject *type, PyObject *name) static int type_update_dict(PyTypeObject *type, PyDictObject *dict, PyObject *name, - PyObject **old_value) + PyObject *value, PyObject **old_value) { // We don't want any re-entrancy between when we update the dict // and call type_modified_unlocked, including running the destructor @@ -5760,7 +5760,7 @@ type_setattro(PyObject *self, PyObject *name, PyObject *value) } BEGIN_TYPE_DICT_LOCK(dict); - res = type_update_dict(type, (PyDictObject *)dict, name, &old_value); + res = type_update_dict(type, (PyDictObject *)dict, name, value, &old_value); assert(_PyType_CheckConsistency(type)); END_TYPE_DICT_LOCK();