diff --git a/Lib/test/test_descr.py b/Lib/test/test_descr.py index a27eb7399dc..3923412b9a0 100644 --- a/Lib/test/test_descr.py +++ b/Lib/test/test_descr.py @@ -5742,7 +5742,6 @@ def __repr__(self): objcopy2 = deepcopy(objcopy) self._assert_is_copy(obj, objcopy2) - @unittest.skip("TODO: RUSTPYTHON") def test_issue24097(self): # Slot name is freed inside __getattr__ and is later used. class S(str): # Not interned diff --git a/crates/vm/src/builtins/object.rs b/crates/vm/src/builtins/object.rs index d7de843a7a0..2e832c571d5 100644 --- a/crates/vm/src/builtins/object.rs +++ b/crates/vm/src/builtins/object.rs @@ -226,16 +226,23 @@ fn object_getstate_default(obj: &PyObject, required: bool, vm: &VirtualMachine) if slot_names_len > 0 { let slots = vm.ctx.new_dict(); for i in 0..slot_names_len { - let borrowed_names = slot_names.borrow_vec(); - // Check if slotnames changed during iteration - if borrowed_names.len() != slot_names_len { + // Release the list lock before the attribute lookup: a getter + // or `__getattr__` may mutate `__slotnames__`. + let name = slot_names.borrow_vec()[i].clone(); + let name = name.downcast::().map_err(|name| { + vm.new_type_error(format!( + "attribute name must be string, not '{}'", + name.class().name() + )) + })?; + if let Some(value) = vm.get_attribute_opt(obj, &name)? { + slots.set_item(name.as_wtf8(), value, vm)?; + } + // The list is stored on the class, so it may change while we + // iterate over it. + if slot_names.borrow_vec().len() != slot_names_len { return Err(vm.new_runtime_error("__slotnames__ changed size during iteration")); } - let name = borrowed_names[i].downcast_ref::().unwrap(); - let Ok(value) = obj.get_attr(name, vm) else { - continue; - }; - slots.set_item(name.as_wtf8(), value, vm).unwrap(); } if !slots.is_empty() { diff --git a/extra_tests/snippets/builtin_object.py b/extra_tests/snippets/builtin_object.py index 9d343eafecb..cfa8a686641 100644 --- a/extra_tests/snippets/builtin_object.py +++ b/extra_tests/snippets/builtin_object.py @@ -193,3 +193,46 @@ def __call__(self, *args): raw_instance.value RawGetDescriptor.__get__ = lambda self, instance, owner: (self, instance, owner) assert raw_instance.value == (raw_descriptor, raw_instance, RawGetOwner) + + +# object.__getstate__ reads names from a cached __slotnames__ list +class NonStrSlotName: + __slotnames__ = [1] + + +with assert_raises(TypeError) as caught: + NonStrSlotName().__getstate__() +assert str(caught.exception) == "attribute name must be string, not 'int'" + + +class GrowingSlotNames: + __slotnames__ = ["a", "b"] + + def __getattr__(self, name): + if name == "a": + GrowingSlotNames.__slotnames__.append("c") + return 1 + raise AttributeError(name) + + +with assert_raises(RuntimeError): + GrowingSlotNames().__getstate__() + + +class RaisingSlot: + __slotnames__ = ["x"] + + def __getattr__(self, name): + raise ValueError("boom") + + +with assert_raises(ValueError): + RaisingSlot().__getstate__() + + +class MissingSlot: + __slotnames__ = ["x", "y"] + y = 2 + + +assert MissingSlot().__getstate__() == (None, {"y": 2})