Skip to content

Commit 7eb0fe4

Browse files
youknowoneyouknowone
authored
Fix potential deadlock (#6509)
1 parent 8443b2c commit 7eb0fe4

File tree

3 files changed

+49
-28
lines changed

3 files changed

+49
-28
lines changed

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

Lines changed: 4 additions & 4 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
}

‎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/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)