From add34a2f1940f920f498bb519a7cb1bc4fa371c6 Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Tue, 17 Mar 2026 21:00:02 +0900 Subject: [PATCH] Drop old PyObjectRef outside type lock to prevent deadlock Dropping values inside with_type_lock can trigger weakref callbacks, which may access attributes (LOAD_ATTR specialization) and re-acquire the non-reentrant type mutex, causing deadlock. Return old values from lock closures so they drop after lock release. --- crates/vm/src/builtins/type.rs | 95 ++++++++++++++++++---------------- 1 file changed, 51 insertions(+), 44 deletions(-) diff --git a/crates/vm/src/builtins/type.rs b/crates/vm/src/builtins/type.rs index 3148ff420..dd937f98f 100644 --- a/crates/vm/src/builtins/type.rs +++ b/crates/vm/src/builtins/type.rs @@ -1562,18 +1562,19 @@ impl PyType { drop(attrs); let none = vm.ctx.none(); - Ok(Self::with_type_lock(vm, || { + let (result, _prev) = Self::with_type_lock(vm, || { let mut attrs = self.attributes.write(); if let Some(annotate) = attrs.get(annotate_key).cloned() { - return annotate; + return (annotate, None); } if let Some(annotate) = attrs.get(annotate_func_key).cloned() { - return annotate; + return (annotate, None); } self.modified_inner(); - attrs.insert(annotate_func_key, none.clone()); - none - })) + let prev = attrs.insert(annotate_func_key, none.clone()); + (none, prev) + }); + Ok(result) } #[pygetset(setter)] @@ -1596,13 +1597,16 @@ impl PyType { return Err(vm.new_type_error("__annotate__ must be callable or None")); } - Self::with_type_lock(vm, || { + let _prev_values = Self::with_type_lock(vm, || { self.modified_inner(); let mut attrs = self.attributes.write(); - if !vm.is_none(&value) { - attrs.swap_remove(identifier!(vm, __annotations_cache__)); - } - attrs.insert(identifier!(vm, __annotate_func__), value); + let removed = if !vm.is_none(&value) { + attrs.swap_remove(identifier!(vm, __annotations_cache__)) + } else { + None + }; + let prev = attrs.insert(identifier!(vm, __annotate_func__), value); + (removed, prev) }); Ok(()) @@ -1665,20 +1669,21 @@ impl PyType { vm.ctx.new_dict().into() }; - Ok(Self::with_type_lock(vm, || { + let (result, _prev) = Self::with_type_lock(vm, || { let mut attrs = self.attributes.write(); if let Some(existing) = attrs.get(annotations_key).cloned() && !existing.class().is(vm.ctx.types.getset_type) { - return existing; + return (existing, None); } if let Some(existing) = attrs.get(annotations_cache_key).cloned() { - return existing; + return (existing, None); } self.modified_inner(); - attrs.insert(annotations_cache_key, annotations.clone()); - annotations - })) + let prev = attrs.insert(annotations_cache_key, annotations.clone()); + (annotations, prev) + }); + Ok(result) } #[pygetset(setter)] @@ -1694,11 +1699,12 @@ impl PyType { ))); } - Self::with_type_lock(vm, || { + let _prev_values = Self::with_type_lock(vm, || { self.modified_inner(); let mut attrs = self.attributes.write(); let has_annotations = attrs.contains_key(identifier!(vm, __annotations__)); + let mut prev = Vec::new(); match value { crate::function::PySetterValue::Assign(value) => { let key = if has_annotations { @@ -1706,32 +1712,29 @@ impl PyType { } else { identifier!(vm, __annotations_cache__) }; - attrs.insert(key, value); + prev.extend(attrs.insert(key, value)); if has_annotations { - attrs.swap_remove(identifier!(vm, __annotations_cache__)); + prev.extend(attrs.swap_remove(identifier!(vm, __annotations_cache__))); } } crate::function::PySetterValue::Delete => { let removed = if has_annotations { - attrs - .swap_remove(identifier!(vm, __annotations__)) - .is_some() + attrs.swap_remove(identifier!(vm, __annotations__)) } else { - attrs - .swap_remove(identifier!(vm, __annotations_cache__)) - .is_some() + attrs.swap_remove(identifier!(vm, __annotations_cache__)) }; - if !removed { + if removed.is_none() { return Err(vm.new_attribute_error("__annotations__")); } + prev.extend(removed); if has_annotations { - attrs.swap_remove(identifier!(vm, __annotations_cache__)); + prev.extend(attrs.swap_remove(identifier!(vm, __annotations_cache__))); } } } - attrs.swap_remove(identifier!(vm, __annotate_func__)); - attrs.swap_remove(identifier!(vm, __annotate__)); - Ok(()) + prev.extend(attrs.swap_remove(identifier!(vm, __annotate_func__))); + prev.extend(attrs.swap_remove(identifier!(vm, __annotate__))); + Ok(prev) })?; Ok(()) @@ -1765,11 +1768,12 @@ impl PyType { #[pygetset(setter)] fn set___module__(&self, value: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { self.check_set_special_type_attr(identifier!(vm, __module__), vm)?; - Self::with_type_lock(vm, || { + let _prev_values = Self::with_type_lock(vm, || { self.modified_inner(); - self.attributes - .write() - .insert(identifier!(vm, __module__), value); + let mut attributes = self.attributes.write(); + let removed = attributes.swap_remove(identifier!(vm, __firstlineno__)); + let prev = attributes.insert(identifier!(vm, __module__), value); + (removed, prev) }); Ok(()) } @@ -1896,9 +1900,9 @@ impl PyType { match value { PySetterValue::Assign(val) => { self.check_set_special_type_attr(key, vm)?; - Self::with_type_lock(vm, || { + let _prev_value = Self::with_type_lock(vm, || { self.modified_inner(); - self.attributes.write().insert(key, val.into()); + self.attributes.write().insert(key, val.into()) }); } PySetterValue::Delete => { @@ -1908,9 +1912,9 @@ impl PyType { self.slot_name() ))); } - Self::with_type_lock(vm, || { + let _prev_value = Self::with_type_lock(vm, || { self.modified_inner(); - self.attributes.write().shift_remove(&key); + self.attributes.write().shift_remove(&key) }); } } @@ -2519,11 +2523,11 @@ impl Py { // Check if we can set this special type attribute self.check_set_special_type_attr(identifier!(vm, __doc__), vm)?; - PyType::with_type_lock(vm, || { + let _prev_value = PyType::with_type_lock(vm, || { self.modified_inner(); self.attributes .write() - .insert(identifier!(vm, __doc__), value); + .insert(identifier!(vm, __doc__), value) }); Ok(()) @@ -2586,14 +2590,17 @@ impl SetAttr for PyType { } let assign = value.is_assign(); - Self::with_type_lock(vm, || { + // Drop old value OUTSIDE the type lock to avoid deadlock: + // dropping may trigger weakref callbacks → method calls → + // LOAD_ATTR specialization → version_for_specialization → type lock. + let _prev_value = Self::with_type_lock(vm, || { // Invalidate inline caches before modifying attributes. // This ensures other threads see the version invalidation before // any attribute changes, preventing use-after-free of cached descriptors. zelf.modified_inner(); if let PySetterValue::Assign(value) = value { - zelf.attributes.write().insert(attr_name, value); + Ok(zelf.attributes.write().insert(attr_name, value)) } else { let prev_value = zelf.attributes.write().shift_remove(attr_name); // TODO: swap_remove applicable? if prev_value.is_none() { @@ -2603,8 +2610,8 @@ impl SetAttr for PyType { attr_name, ))); } + Ok(prev_value) } - Ok(()) })?; if attr_name.as_wtf8().starts_with("__") && attr_name.as_wtf8().ends_with("__") {