Skip to content

Commit 8276cf6

Browse files
youknowoneyouknowone
committed
Fix potential deadlocks
1 parent aae6bf5 commit 8276cf6

File tree

4 files changed

+95
-50
lines changed

4 files changed

+95
-50
lines changed

‎crates/vm/src/builtins/classmethod.rs‎

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -57,11 +57,11 @@ impl GetDescriptor for PyClassMethod {
5757
) -> PyResult {
5858
let (zelf, _obj) = Self::_unwrap(&zelf, obj, vm)?;
5959
let cls = cls.unwrap_or_else(|| _obj.class().to_owned().into());
60-
let call_descr_get: PyResult<PyObjectRef> = zelf.callable.lock().get_attr("__get__", vm);
60+
// Clone and release lock before calling Python code to prevent deadlock
61+
let callable = zelf.callable.lock().clone();
62+
let call_descr_get: PyResult<PyObjectRef> = callable.get_attr("__get__", vm);
6163
match call_descr_get {
62-
Err(_) => Ok(PyBoundMethod::new(cls, zelf.callable.lock().clone())
63-
.into_ref(&vm.ctx)
64-
.into()),
64+
Err(_) => Ok(PyBoundMethod::new(cls, callable).into_ref(&vm.ctx).into()),
6565
Ok(call_descr_get) => call_descr_get.call((cls.clone(), cls), vm),
6666
}
6767
}
@@ -140,37 +140,47 @@ impl PyClassMethod {
140140

141141
#[pygetset]
142142
fn __module__(&self, vm: &VirtualMachine) -> PyResult {
143-
self.callable.lock().get_attr("__module__", vm)
143+
// Clone and release lock before calling Python code to prevent deadlock
144+
let callable = self.callable.lock().clone();
145+
callable.get_attr("__module__", vm)
144146
}
145147

146148
#[pygetset]
147149
fn __qualname__(&self, vm: &VirtualMachine) -> PyResult {
148-
self.callable.lock().get_attr("__qualname__", vm)
150+
// Clone and release lock before calling Python code to prevent deadlock
151+
let callable = self.callable.lock().clone();
152+
callable.get_attr("__qualname__", vm)
149153
}
150154

151155
#[pygetset]
152156
fn __name__(&self, vm: &VirtualMachine) -> PyResult {
153-
self.callable.lock().get_attr("__name__", vm)
157+
// Clone and release lock before calling Python code to prevent deadlock
158+
let callable = self.callable.lock().clone();
159+
callable.get_attr("__name__", vm)
154160
}
155161

156162
#[pygetset]
157163
fn __annotations__(&self, vm: &VirtualMachine) -> PyResult {
158-
self.callable.lock().get_attr("__annotations__", vm)
164+
// Clone and release lock before calling Python code to prevent deadlock
165+
let callable = self.callable.lock().clone();
166+
callable.get_attr("__annotations__", vm)
159167
}
160168

161169
#[pygetset]
162170
fn __isabstractmethod__(&self, vm: &VirtualMachine) -> PyObjectRef {
163-
match vm.get_attribute_opt(self.callable.lock().clone(), "__isabstractmethod__") {
171+
// Clone and release lock before calling Python code to prevent deadlock
172+
let callable = self.callable.lock().clone();
173+
match vm.get_attribute_opt(callable, "__isabstractmethod__") {
164174
Ok(Some(is_abstract)) => is_abstract,
165175
_ => vm.ctx.new_bool(false).into(),
166176
}
167177
}
168178

169179
#[pygetset(setter)]
170180
fn set___isabstractmethod__(&self, value: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
171-
self.callable
172-
.lock()
173-
.set_attr("__isabstractmethod__", value, vm)?;
181+
// Clone and release lock before calling Python code to prevent deadlock
182+
let callable = self.callable.lock().clone();
183+
callable.set_attr("__isabstractmethod__", value, vm)?;
174184
Ok(())
175185
}
176186

@@ -183,7 +193,9 @@ impl PyClassMethod {
183193
impl Representable for PyClassMethod {
184194
#[inline]
185195
fn repr_str(zelf: &Py<Self>, vm: &VirtualMachine) -> PyResult<String> {
186-
let callable = zelf.callable.lock().repr(vm).unwrap();
196+
// Clone and release lock before calling Python code to prevent deadlock
197+
let callable = zelf.callable.lock().clone();
198+
let callable_repr = callable.repr(vm).unwrap();
187199
let class = Self::class(&vm.ctx);
188200

189201
let repr = match (
@@ -198,9 +210,9 @@ impl Representable for PyClassMethod {
198210
) {
199211
(None, _) => return Err(vm.new_type_error("Unknown qualified name")),
200212
(Some(qualname), Some(module)) if module != "builtins" => {
201-
format!("<{module}.{qualname}({callable})>")
213+
format!("<{module}.{qualname}({callable_repr})>")
202214
}
203-
_ => format!("<{}({})>", class.slot_name(), callable),
215+
_ => format!("<{}({})>", class.slot_name(), callable_repr),
204216
};
205217
Ok(repr)
206218
}

‎crates/vm/src/builtins/property.rs‎

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,8 @@ impl GetDescriptor for PyProperty {
5555
let (zelf, obj) = Self::_unwrap(&zelf_obj, obj, vm)?;
5656
if vm.is_none(&obj) {
5757
Ok(zelf_obj)
58-
} else if let Some(getter) = zelf.getter.read().as_ref() {
58+
} else if let Some(getter) = zelf.getter.read().clone() {
59+
// Clone and release lock before calling Python code to prevent deadlock
5960
getter.call((obj,), vm)
6061
} else {
6162
let error_msg = zelf.format_property_error(&obj, "getter", vm)?;
@@ -70,12 +71,12 @@ impl PyProperty {
7071
// Returns the name if available, None if not found, or propagates errors
7172
fn get_property_name(&self, vm: &VirtualMachine) -> PyResult<Option<PyObjectRef>> {
7273
// First check if name was set via __set_name__
73-
if let Some(name) = self.name.read().as_ref() {
74-
return Ok(Some(name.clone()));
74+
if let Some(name) = self.name.read().clone() {
75+
return Ok(Some(name));
7576
}
7677

77-
let getter = self.getter.read();
78-
let Some(getter) = getter.as_ref() else {
78+
// Clone and release lock before calling Python code to prevent deadlock
79+
let Some(getter) = self.getter.read().clone() else {
7980
return Ok(None);
8081
};
8182

@@ -105,15 +106,17 @@ impl PyProperty {
105106
let zelf = zelf.try_to_ref::<Self>(vm)?;
106107
match value {
107108
PySetterValue::Assign(value) => {
108-
if let Some(setter) = zelf.setter.read().as_ref() {
109+
// Clone and release lock before calling Python code to prevent deadlock
110+
if let Some(setter) = zelf.setter.read().clone() {
109111
setter.call((obj, value), vm).map(drop)
110112
} else {
111113
let error_msg = zelf.format_property_error(&obj, "setter", vm)?;
112114
Err(vm.new_attribute_error(error_msg))
113115
}
114116
}
115117
PySetterValue::Delete => {
116-
if let Some(deleter) = zelf.deleter.read().as_ref() {
118+
// Clone and release lock before calling Python code to prevent deadlock
119+
if let Some(deleter) = zelf.deleter.read().clone() {
117120
deleter.call((obj,), vm).map(drop)
118121
} else {
119122
let error_msg = zelf.format_property_error(&obj, "deleter", vm)?;
@@ -273,23 +276,24 @@ impl PyProperty {
273276
}
274277
};
275278

279+
// Clone and release lock before calling Python code to prevent deadlock
276280
// Check getter
277-
if let Some(getter) = self.getter.read().as_ref()
278-
&& is_abstract(getter)?
281+
if let Some(getter) = self.getter.read().clone()
282+
&& is_abstract(&getter)?
279283
{
280284
return Ok(vm.ctx.new_bool(true).into());
281285
}
282286

283287
// Check setter
284-
if let Some(setter) = self.setter.read().as_ref()
285-
&& is_abstract(setter)?
288+
if let Some(setter) = self.setter.read().clone()
289+
&& is_abstract(&setter)?
286290
{
287291
return Ok(vm.ctx.new_bool(true).into());
288292
}
289293

290294
// Check deleter
291-
if let Some(deleter) = self.deleter.read().as_ref()
292-
&& is_abstract(deleter)?
295+
if let Some(deleter) = self.deleter.read().clone()
296+
&& is_abstract(&deleter)?
293297
{
294298
return Ok(vm.ctx.new_bool(true).into());
295299
}
@@ -299,7 +303,8 @@ impl PyProperty {
299303

300304
#[pygetset(setter)]
301305
fn set___isabstractmethod__(&self, value: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
302-
if let Some(getter) = self.getter.read().to_owned() {
306+
// Clone and release lock before calling Python code to prevent deadlock
307+
if let Some(getter) = self.getter.read().clone() {
303308
getter.set_attr("__isabstractmethod__", value, vm)?;
304309
}
305310
Ok(())

‎crates/vm/src/builtins/staticmethod.rs‎

Lines changed: 23 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -103,37 +103,47 @@ impl PyStaticMethod {
103103

104104
#[pygetset]
105105
fn __module__(&self, vm: &VirtualMachine) -> PyResult {
106-
self.callable.lock().get_attr("__module__", vm)
106+
// Clone and release lock before calling Python code to prevent deadlock
107+
let callable = self.callable.lock().clone();
108+
callable.get_attr("__module__", vm)
107109
}
108110

109111
#[pygetset]
110112
fn __qualname__(&self, vm: &VirtualMachine) -> PyResult {
111-
self.callable.lock().get_attr("__qualname__", vm)
113+
// Clone and release lock before calling Python code to prevent deadlock
114+
let callable = self.callable.lock().clone();
115+
callable.get_attr("__qualname__", vm)
112116
}
113117

114118
#[pygetset]
115119
fn __name__(&self, vm: &VirtualMachine) -> PyResult {
116-
self.callable.lock().get_attr("__name__", vm)
120+
// Clone and release lock before calling Python code to prevent deadlock
121+
let callable = self.callable.lock().clone();
122+
callable.get_attr("__name__", vm)
117123
}
118124

119125
#[pygetset]
120126
fn __annotations__(&self, vm: &VirtualMachine) -> PyResult {
121-
self.callable.lock().get_attr("__annotations__", vm)
127+
// Clone and release lock before calling Python code to prevent deadlock
128+
let callable = self.callable.lock().clone();
129+
callable.get_attr("__annotations__", vm)
122130
}
123131

124132
#[pygetset]
125133
fn __isabstractmethod__(&self, vm: &VirtualMachine) -> PyObjectRef {
126-
match vm.get_attribute_opt(self.callable.lock().clone(), "__isabstractmethod__") {
134+
// Clone and release lock before calling Python code to prevent deadlock
135+
let callable = self.callable.lock().clone();
136+
match vm.get_attribute_opt(callable, "__isabstractmethod__") {
127137
Ok(Some(is_abstract)) => is_abstract,
128138
_ => vm.ctx.new_bool(false).into(),
129139
}
130140
}
131141

132142
#[pygetset(setter)]
133143
fn set___isabstractmethod__(&self, value: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
134-
self.callable
135-
.lock()
136-
.set_attr("__isabstractmethod__", value, vm)?;
144+
// Clone and release lock before calling Python code to prevent deadlock
145+
let callable = self.callable.lock().clone();
146+
callable.set_attr("__isabstractmethod__", value, vm)?;
137147
Ok(())
138148
}
139149

@@ -154,7 +164,9 @@ impl Callable for PyStaticMethod {
154164

155165
impl Representable for PyStaticMethod {
156166
fn repr_str(zelf: &Py<Self>, vm: &VirtualMachine) -> PyResult<String> {
157-
let callable = zelf.callable.lock().repr(vm).unwrap();
167+
// Clone and release lock before calling Python code to prevent deadlock
168+
let callable = zelf.callable.lock().clone();
169+
let callable_repr = callable.repr(vm).unwrap();
158170
let class = Self::class(&vm.ctx);
159171

160172
match (
@@ -169,9 +181,9 @@ impl Representable for PyStaticMethod {
169181
) {
170182
(None, _) => Err(vm.new_type_error("Unknown qualified name")),
171183
(Some(qualname), Some(module)) if module != "builtins" => {
172-
Ok(format!("<{module}.{qualname}({callable})>"))
184+
Ok(format!("<{module}.{qualname}({callable_repr})>"))
173185
}
174-
_ => Ok(format!("<{}({})>", class.slot_name(), callable)),
186+
_ => Ok(format!("<{}({})>", class.slot_name(), callable_repr)),
175187
}
176188
}
177189
}

‎crates/vm/src/stdlib/functools.rs‎

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -248,15 +248,24 @@ mod _functools {
248248
type Args = FuncArgs;
249249

250250
fn call(zelf: &Py<Self>, args: FuncArgs, vm: &VirtualMachine) -> PyResult {
251-
let inner = zelf.inner.read();
252-
let mut combined_args = inner.args.as_slice().to_vec();
251+
// Clone and release lock before calling Python code to prevent deadlock
252+
let (func, stored_args, keywords) = {
253+
let inner = zelf.inner.read();
254+
(
255+
inner.func.clone(),
256+
inner.args.clone(),
257+
inner.keywords.clone(),
258+
)
259+
};
260+
261+
let mut combined_args = stored_args.as_slice().to_vec();
253262
combined_args.extend_from_slice(&args.args);
254263

255264
// Merge keywords from self.keywords and args.kwargs
256265
let mut final_kwargs = IndexMap::new();
257266

258267
// Add keywords from self.keywords
259-
for (key, value) in &*inner.keywords {
268+
for (key, value) in &*keywords {
260269
let key_str = key
261270
.downcast::<crate::builtins::PyStr>()
262271
.map_err(|_| vm.new_type_error("keywords must be strings"))?;
@@ -268,9 +277,7 @@ mod _functools {
268277
final_kwargs.insert(key, value);
269278
}
270279

271-
inner
272-
.func
273-
.call(FuncArgs::new(combined_args, KwArgs::new(final_kwargs)), vm)
280+
func.call(FuncArgs::new(combined_args, KwArgs::new(final_kwargs)), vm)
274281
}
275282
}
276283

@@ -280,15 +287,24 @@ mod _functools {
280287
// Check for recursive repr
281288
let obj = zelf.as_object();
282289
if let Some(_guard) = ReprGuard::enter(vm, obj) {
283-
let inner = zelf.inner.read();
284-
let func_repr = inner.func.repr(vm)?;
290+
// Clone and release lock before calling Python code to prevent deadlock
291+
let (func, args, keywords) = {
292+
let inner = zelf.inner.read();
293+
(
294+
inner.func.clone(),
295+
inner.args.clone(),
296+
inner.keywords.clone(),
297+
)
298+
};
299+
300+
let func_repr = func.repr(vm)?;
285301
let mut parts = vec![func_repr.as_str().to_owned()];
286302

287-
for arg in inner.args.as_slice() {
303+
for arg in args.as_slice() {
288304
parts.push(arg.repr(vm)?.as_str().to_owned());
289305
}
290306

291-
for (key, value) in inner.keywords.clone() {
307+
for (key, value) in &*keywords {
292308
// For string keys, use them directly without quotes
293309
let key_part = if let Ok(s) = key.clone().downcast::<crate::builtins::PyStr>() {
294310
s.as_str().to_owned()

0 commit comments

Comments
 (0)