Skip to content

Commit add34a2

Browse files
committed
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.
1 parent e0886e2 commit add34a2

File tree

1 file changed

+51
-44
lines changed

1 file changed

+51
-44
lines changed

crates/vm/src/builtins/type.rs

Lines changed: 51 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -1562,18 +1562,19 @@ impl PyType {
15621562
drop(attrs);
15631563

15641564
let none = vm.ctx.none();
1565-
Ok(Self::with_type_lock(vm, || {
1565+
let (result, _prev) = Self::with_type_lock(vm, || {
15661566
let mut attrs = self.attributes.write();
15671567
if let Some(annotate) = attrs.get(annotate_key).cloned() {
1568-
return annotate;
1568+
return (annotate, None);
15691569
}
15701570
if let Some(annotate) = attrs.get(annotate_func_key).cloned() {
1571-
return annotate;
1571+
return (annotate, None);
15721572
}
15731573
self.modified_inner();
1574-
attrs.insert(annotate_func_key, none.clone());
1575-
none
1576-
}))
1574+
let prev = attrs.insert(annotate_func_key, none.clone());
1575+
(none, prev)
1576+
});
1577+
Ok(result)
15771578
}
15781579

15791580
#[pygetset(setter)]
@@ -1596,13 +1597,16 @@ impl PyType {
15961597
return Err(vm.new_type_error("__annotate__ must be callable or None"));
15971598
}
15981599

1599-
Self::with_type_lock(vm, || {
1600+
let _prev_values = Self::with_type_lock(vm, || {
16001601
self.modified_inner();
16011602
let mut attrs = self.attributes.write();
1602-
if !vm.is_none(&value) {
1603-
attrs.swap_remove(identifier!(vm, __annotations_cache__));
1604-
}
1605-
attrs.insert(identifier!(vm, __annotate_func__), value);
1603+
let removed = if !vm.is_none(&value) {
1604+
attrs.swap_remove(identifier!(vm, __annotations_cache__))
1605+
} else {
1606+
None
1607+
};
1608+
let prev = attrs.insert(identifier!(vm, __annotate_func__), value);
1609+
(removed, prev)
16061610
});
16071611

16081612
Ok(())
@@ -1665,20 +1669,21 @@ impl PyType {
16651669
vm.ctx.new_dict().into()
16661670
};
16671671

1668-
Ok(Self::with_type_lock(vm, || {
1672+
let (result, _prev) = Self::with_type_lock(vm, || {
16691673
let mut attrs = self.attributes.write();
16701674
if let Some(existing) = attrs.get(annotations_key).cloned()
16711675
&& !existing.class().is(vm.ctx.types.getset_type)
16721676
{
1673-
return existing;
1677+
return (existing, None);
16741678
}
16751679
if let Some(existing) = attrs.get(annotations_cache_key).cloned() {
1676-
return existing;
1680+
return (existing, None);
16771681
}
16781682
self.modified_inner();
1679-
attrs.insert(annotations_cache_key, annotations.clone());
1680-
annotations
1681-
}))
1683+
let prev = attrs.insert(annotations_cache_key, annotations.clone());
1684+
(annotations, prev)
1685+
});
1686+
Ok(result)
16821687
}
16831688

16841689
#[pygetset(setter)]
@@ -1694,44 +1699,42 @@ impl PyType {
16941699
)));
16951700
}
16961701

1697-
Self::with_type_lock(vm, || {
1702+
let _prev_values = Self::with_type_lock(vm, || {
16981703
self.modified_inner();
16991704
let mut attrs = self.attributes.write();
17001705
let has_annotations = attrs.contains_key(identifier!(vm, __annotations__));
17011706

1707+
let mut prev = Vec::new();
17021708
match value {
17031709
crate::function::PySetterValue::Assign(value) => {
17041710
let key = if has_annotations {
17051711
identifier!(vm, __annotations__)
17061712
} else {
17071713
identifier!(vm, __annotations_cache__)
17081714
};
1709-
attrs.insert(key, value);
1715+
prev.extend(attrs.insert(key, value));
17101716
if has_annotations {
1711-
attrs.swap_remove(identifier!(vm, __annotations_cache__));
1717+
prev.extend(attrs.swap_remove(identifier!(vm, __annotations_cache__)));
17121718
}
17131719
}
17141720
crate::function::PySetterValue::Delete => {
17151721
let removed = if has_annotations {
1716-
attrs
1717-
.swap_remove(identifier!(vm, __annotations__))
1718-
.is_some()
1722+
attrs.swap_remove(identifier!(vm, __annotations__))
17191723
} else {
1720-
attrs
1721-
.swap_remove(identifier!(vm, __annotations_cache__))
1722-
.is_some()
1724+
attrs.swap_remove(identifier!(vm, __annotations_cache__))
17231725
};
1724-
if !removed {
1726+
if removed.is_none() {
17251727
return Err(vm.new_attribute_error("__annotations__"));
17261728
}
1729+
prev.extend(removed);
17271730
if has_annotations {
1728-
attrs.swap_remove(identifier!(vm, __annotations_cache__));
1731+
prev.extend(attrs.swap_remove(identifier!(vm, __annotations_cache__)));
17291732
}
17301733
}
17311734
}
1732-
attrs.swap_remove(identifier!(vm, __annotate_func__));
1733-
attrs.swap_remove(identifier!(vm, __annotate__));
1734-
Ok(())
1735+
prev.extend(attrs.swap_remove(identifier!(vm, __annotate_func__)));
1736+
prev.extend(attrs.swap_remove(identifier!(vm, __annotate__)));
1737+
Ok(prev)
17351738
})?;
17361739

17371740
Ok(())
@@ -1765,11 +1768,12 @@ impl PyType {
17651768
#[pygetset(setter)]
17661769
fn set___module__(&self, value: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
17671770
self.check_set_special_type_attr(identifier!(vm, __module__), vm)?;
1768-
Self::with_type_lock(vm, || {
1771+
let _prev_values = Self::with_type_lock(vm, || {
17691772
self.modified_inner();
1770-
self.attributes
1771-
.write()
1772-
.insert(identifier!(vm, __module__), value);
1773+
let mut attributes = self.attributes.write();
1774+
let removed = attributes.swap_remove(identifier!(vm, __firstlineno__));
1775+
let prev = attributes.insert(identifier!(vm, __module__), value);
1776+
(removed, prev)
17731777
});
17741778
Ok(())
17751779
}
@@ -1896,9 +1900,9 @@ impl PyType {
18961900
match value {
18971901
PySetterValue::Assign(val) => {
18981902
self.check_set_special_type_attr(key, vm)?;
1899-
Self::with_type_lock(vm, || {
1903+
let _prev_value = Self::with_type_lock(vm, || {
19001904
self.modified_inner();
1901-
self.attributes.write().insert(key, val.into());
1905+
self.attributes.write().insert(key, val.into())
19021906
});
19031907
}
19041908
PySetterValue::Delete => {
@@ -1908,9 +1912,9 @@ impl PyType {
19081912
self.slot_name()
19091913
)));
19101914
}
1911-
Self::with_type_lock(vm, || {
1915+
let _prev_value = Self::with_type_lock(vm, || {
19121916
self.modified_inner();
1913-
self.attributes.write().shift_remove(&key);
1917+
self.attributes.write().shift_remove(&key)
19141918
});
19151919
}
19161920
}
@@ -2519,11 +2523,11 @@ impl Py<PyType> {
25192523
// Check if we can set this special type attribute
25202524
self.check_set_special_type_attr(identifier!(vm, __doc__), vm)?;
25212525

2522-
PyType::with_type_lock(vm, || {
2526+
let _prev_value = PyType::with_type_lock(vm, || {
25232527
self.modified_inner();
25242528
self.attributes
25252529
.write()
2526-
.insert(identifier!(vm, __doc__), value);
2530+
.insert(identifier!(vm, __doc__), value)
25272531
});
25282532

25292533
Ok(())
@@ -2586,14 +2590,17 @@ impl SetAttr for PyType {
25862590
}
25872591
let assign = value.is_assign();
25882592

2589-
Self::with_type_lock(vm, || {
2593+
// Drop old value OUTSIDE the type lock to avoid deadlock:
2594+
// dropping may trigger weakref callbacks → method calls →
2595+
// LOAD_ATTR specialization → version_for_specialization → type lock.
2596+
let _prev_value = Self::with_type_lock(vm, || {
25902597
// Invalidate inline caches before modifying attributes.
25912598
// This ensures other threads see the version invalidation before
25922599
// any attribute changes, preventing use-after-free of cached descriptors.
25932600
zelf.modified_inner();
25942601

25952602
if let PySetterValue::Assign(value) = value {
2596-
zelf.attributes.write().insert(attr_name, value);
2603+
Ok(zelf.attributes.write().insert(attr_name, value))
25972604
} else {
25982605
let prev_value = zelf.attributes.write().shift_remove(attr_name); // TODO: swap_remove applicable?
25992606
if prev_value.is_none() {
@@ -2603,8 +2610,8 @@ impl SetAttr for PyType {
26032610
attr_name,
26042611
)));
26052612
}
2613+
Ok(prev_value)
26062614
}
2607-
Ok(())
26082615
})?;
26092616

26102617
if attr_name.as_wtf8().starts_with("__") && attr_name.as_wtf8().ends_with("__") {

0 commit comments

Comments
 (0)