RustPython: bug: Double Free On `BufferedReader`&`FileIO`?
Bug behavior
Added a is_drop field in PyInner. and found out both BufferedReader&FileIO gets double drop? Did some search on the code, no clue where misses a clone() to PyRef or PyObjectRef
Possible root of bug
a obscure memcpy on a struct containing a PyRef/PyObjectRef
Necessity of fixing the bug
Useful for writing garbage collecting, also prevent UB
Part of the log
the remaining of log file just basically repeats those lines. The command is just cargo run, and alright it did run, but with all those double drop:
log_buf.log
[ERROR rustpython_vm::object::core] Double drop on PyRef, type="rustpython_vm::stdlib::io::_io::BufferedReader"
[ERROR rustpython_vm::object::core] Double drop on PyRef, type="rustpython_vm::stdlib::io::fileio::FileIO"
[ERROR rustpython_vm::object::core] Double drop on PyObjectRef, typeid=TypeId { t: 14576741135102236696 }, type=Some("rustpython_vm::stdlib::io::fileio::FileIO")
[ERROR rustpython_vm::object::core] Double drop on PyRef, type="rustpython_vm::stdlib::io::_io::BufferedReader"
backtrace= 0: <rustpython_vm::object::core::PyRef<T> as core::ops::drop::Drop>::drop
at vm/src/object/core.rs:979:17
1: core::ptr::drop_in_place<rustpython_vm::object::core::PyRef<rustpython_vm::stdlib::io::_io::BufferedReader>>
at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/ptr/mod.rs:486:1
2: rustpython_vm::stdlib::io::_io::BufferedMixin::close
at vm/src/stdlib/io.rs:1575:9
3: core::ops::function::Fn::call
at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/ops/function.rs:77:5
4: rustpython_vm::function::builtin::<impl rustpython_vm::function::builtin::sealed::PyNativeFuncInternal<(rustpython_vm::function::builtin::OwnedParam<T1>,),R,rustpython_vm::vm::VirtualMachine> for F>::call_
at vm/src/function/builtin.rs:79:17
5: <F as rustpython_vm::function::builtin::IntoPyNativeFunc<(T,R,VM)>>::call
at vm/src/function/builtin.rs:47:9
rustpython_vm::function::builtin::IntoPyNativeFunc::into_func::{{closure}}
at vm/src/function/builtin.rs:35:51
6: <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call
at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/alloc/src/boxed.rs:1886:9
7: <rustpython_vm::builtins::builtinfunc::PyBuiltinMethod as rustpython_vm::types::slot::Callable>::call
at vm/src/builtins/builtinfunc.rs:210:9
8: rustpython_vm::types::slot::Callable::slot_call
at vm/src/types/slot.rs:562:13
9: rustpython_vm::vm::vm_object::<impl rustpython_vm::vm::VirtualMachine>::_invoke
at vm/src/vm/vm_object.rs:176:30
10: rustpython_vm::vm::vm_object::<impl rustpython_vm::vm::VirtualMachine>::invoke
at vm/src/vm/vm_object.rs:189:9
rustpython_vm::vm::method::PyMethod::invoke
at vm/src/vm/method.rs:121:9
11: rustpython_vm::vm::vm_object::<impl rustpython_vm::vm::VirtualMachine>::call_method
at vm/src/vm/vm_object.rs:127:9
12: <rustpython_vm::stdlib::io::_io::_IOBase as rustpython_vm::types::slot::Destructor>::slot_del
at vm/src/stdlib/io.rs:541:21
13: rustpython_vm::object::core::PyObject::drop_slow_inner::call_slot_del::{{closure}}
at vm/src/object/core.rs:768:33
14: rustpython_vm::vm::thread::with_vm::{{closure}}
at vm/src/vm/thread.rs:38:14
15: std::thread::local::LocalKey<T>::try_with
at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/thread/local.rs:445:16
16: std::thread::local::LocalKey<T>::with
at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/thread/local.rs:421:9
17: rustpython_vm::vm::thread::with_vm
at vm/src/vm/thread.rs:27:5
18: rustpython_vm::object::core::PyObject::drop_slow_inner::call_slot_del
at vm/src/object/core.rs:766:23
19: rustpython_vm::object::core::PyObject::drop_slow_inner
at vm/src/object/core.rs:789:13
rustpython_vm::object::core::PyObject::drop_slow
at vm/src/object/core.rs:801:26
20: <rustpython_vm::object::core::PyObjectRef as core::ops::drop::Drop>::drop
at vm/src/object/core.rs:871:22
21: core::ptr::drop_in_place<rustpython_vm::object::core::PyObjectRef>
at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/ptr/mod.rs:486:1
22: core::ptr::drop_in_place<core::option::Option<rustpython_vm::object::core::PyObjectRef>>
at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/ptr/mod.rs:486:1
23: core::ptr::drop_in_place<[core::option::Option<rustpython_vm::object::core::PyObjectRef>]>
at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/ptr/mod.rs:486:1
24: core::ptr::drop_in_place<alloc::boxed::Box<[core::option::Option<rustpython_vm::object::core::PyObjectRef>]>>
at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/ptr/mod.rs:486:1
25: core::ptr::drop_in_place<core::cell::UnsafeCell<alloc::boxed::Box<[core::option::Option<rustpython_vm::object::core::PyObjectRef>]>>>
at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/ptr/mod.rs:486:1
26: core::ptr::drop_in_place<lock_api::mutex::Mutex<parking_lot::raw_mutex::RawMutex,alloc::boxed::Box<[core::option::Option<rustpython_vm::object::core::PyObjectRef>]>>>
at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/ptr/mod.rs:486:1
27: core::ptr::drop_in_place<rustpython_vm::frame::Frame>
at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/ptr/mod.rs:486:1
28: core::ptr::drop_in_place<rustpython_vm::object::core::PyInner<rustpython_vm::frame::Frame>>
at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/ptr/mod.rs:486:1
29: core::ptr::drop_in_place<alloc::boxed::Box<rustpython_vm::object::core::PyInner<rustpython_vm::frame::Frame>>>
at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/ptr/mod.rs:486:1
30: core::mem::drop
at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/mem/mod.rs:974:24
31: rustpython_vm::object::core::drop_dealloc_obj
at vm/src/object/core.rs:89:5
32: rustpython_vm::object::core::PyObject::drop_slow
at vm/src/object/core.rs:807:9
Test Code
added is_drop field to PyInner<T> in core.rs
This is the git diff patch file(Also added a backtrace crate for debugging in this patch)
And here is core code to check double dropping:
diff --git a/vm/src/object/core.rs b/vm/src/object/core.rs
index 19055df25..0eed91f4a 100644
--- a/vm/src/object/core.rs
+++ b/vm/src/object/core.rs
@@ -31,6 +31,7 @@ use std::{
any::TypeId,
borrow::Borrow,
cell::UnsafeCell,
+ collections::HashMap,
fmt,
marker::PhantomData,
mem::ManuallyDrop,
@@ -38,6 +39,11 @@ use std::{
ptr::{self, NonNull},
};
+use once_cell::sync::Lazy;
+
+pub static ID2TYPE: Lazy<PyMutex<HashMap<TypeId, String>>> =
+ Lazy::new(|| PyMutex::new(HashMap::new()));
+
// so, PyObjectRef is basically equivalent to `PyRc<PyInner<dyn PyObjectPayload>>`, except it's
// only one pointer in width rather than 2. We do that by manually creating a vtable, and putting
// a &'static reference to it inside the `PyRc` rather than adjacent to it, like trait objects do.
@@ -109,6 +115,7 @@ impl PyObjVTable {
#[repr(C)]
struct PyInner<T> {
ref_count: RefCount,
+ is_drop: PyMutex<bool>,
// TODO: move typeid into vtable once TypeId::of is const
typeid: TypeId,
vtable: &'static PyObjVTable,
@@ -433,6 +440,7 @@ impl<T: PyObjectPayload> PyInner<T> {
let member_count = typ.slots.member_count;
Box::new(PyInner {
ref_count: RefCount::new(),
+ is_drop: PyMutex::new(false),
typeid: TypeId::of::<T>(),
vtable: PyObjVTable::of::<T>(),
typ: PyAtomicRef::from(typ),
@@ -849,7 +857,15 @@ impl<'a, T: PyObjectPayload> From<&'a Py<T>> for &'a PyObject {
impl Drop for PyObjectRef {
#[inline]
fn drop(&mut self) {
+ if *self.0.is_drop.lock() {
+ error!(
+ "Double drop on PyObjectRef, typeid={:?}, type={:?}",
+ self.0.typeid,
+ ID2TYPE.lock().get(&self.0.typeid)
+ );
+ }
if self.0.ref_count.dec() {
+ *self.0.is_drop.lock() = true;
unsafe { PyObject::drop_slow(self.ptr) }
}
}
@@ -953,7 +969,21 @@ impl<T: PyObjectPayload> fmt::Debug for PyRef<T> {
impl<T: PyObjectPayload> Drop for PyRef<T> {
#[inline]
fn drop(&mut self) {
+ if *self.as_object().0.is_drop.lock() {
+ error!(
+ "Double drop on PyRef, type={:?}",
+ std::any::type_name::<T>()
+ );
+ }
+
+ let tid = TypeId::of::<T>();
+ ID2TYPE
+ .lock()
+ .entry(tid)
+ .or_insert_with(|| std::any::type_name::<T>().to_string());
+
if self.0.ref_count.dec() {
+ *self.0.is_drop.lock() = true;
unsafe { PyObject::drop_slow(self.ptr.cast::<PyObject>()) }
}
}
@@ -1136,6 +1166,7 @@ pub(crate) fn init_type_hierarchy() -> (PyTypeRef, PyTypeRef, PyTypeRef) {
let type_type_ptr = Box::into_raw(Box::new(partially_init!(
PyInner::<PyType> {
ref_count: RefCount::new(),
+ is_drop: PyMutex::new(false),
typeid: TypeId::of::<PyType>(),
vtable: PyObjVTable::of::<PyType>(),
dict: None,
@@ -1148,6 +1179,7 @@ pub(crate) fn init_type_hierarchy() -> (PyTypeRef, PyTypeRef, PyTypeRef) {
let object_type_ptr = Box::into_raw(Box::new(partially_init!(
PyInner::<PyType> {
ref_count: RefCount::new(),
+ is_drop: PyMutex::new(false),
typeid: TypeId::of::<PyType>(),
vtable: PyObjVTable::of::<PyType>(),
dict: None,
About this issue
- Original URL
- State: closed
- Created 2 years ago
- Comments: 18 (18 by maintainers)
This is by no means my final solution, but I found if I elide a drop call in _IOBase then everything is going to be fine, no double drop or whatever (although I don’t know whether there is leak in anywhere):
Edited: eliding
instance’drop() in_IOBase’sflushalso “works”There is one extra call to
BufferedMixin’sclose()from Frame’s fastlocal’s drop, but still can’t figure out why would the ref count be missingThe first deref of a PyObjectRef/PyRef to a dropped object is from
_IOBase’sslot_delfunction, which, I assume, run _IOBase’s__del__fn for BufferedReader? I think I might know what’s wrong in here…It seems https://github.com/RustPython/RustPython/blob/426f9f6359dc5f8bef914693785dc16d345fa66b/vm/src/stdlib/io.rs#L675 this line might cause missing ref count, if: the struct being copied contains fields that’s
PyObjectRef/PyRefthen the recorded ref count would be smaller than real ref count by one. This line was written two years ago, when our ref count system haven’t being built, gonna experiment a little more with those lines to confirm if this guess is right? @youknowone Edited: to fix that, might need toTraceevery PyObjectRef/PyRef a data type own, and call clone for them, thenManuallyDrop& forgot them? Edited1: still bugged on double dropBufferedReadereven after manually clone and inc ref count for PyBuffer Edited2: found out seems onlyPyBytesuse thiscopy_from_sliceand it doesn’t ownPyObjectRef/PyRefcheck.patch.txt I apply this patch to the earliest commit that core.rs exist, and still found this bug, I think this trace back long way ago