New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bug: Double Free On BufferedReader
&FileIO
?
#4238
Comments
BufferedReader
&FileIO
?BufferedReader
&FileIO
?
Double |
Added backtrace on every double drop, yes it seems mostly from fastlocals? This is the new log file: |
Do you think it is a good idea to add double drop checker in debug mode, if so, I might want to make a PR about that? |
Sure, that will be helpful prevent it. I guess we can check similar bug with miri, but running miri is not easy as running debug mode. |
come up with #4242 (ah, double the answer to life, universe and everything that gets drop XD) |
check.patch.txt
|
io's classes differs from most of other rust-implemented types in RustPython because they use inheritance between rust-implemented types. maybe we made mistake somewhere around the point. |
one thing to check? is it called from once as FileIO and another once as _RawIOBase? |
From the backtrace of drop, I can see some is from |
I do notice several copy_from_slice in io.rs I wonder if they might be the cause of this bug? Edited: could be the problem of |
It seems RustPython/vm/src/stdlib/io.rs Line 675 in 426f9f6
this line might cause missing ref count, if: the struct being copied contains fields that's PyObjectRef /PyRef then 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 to Trace every PyObjectRef/PyRef a data type own, and call clone for them, then ManuallyDrop & forgot them?Edited1: still bugged on double drop BufferedReader even after manually clone and inc ref count for PyBufferEdited2: found out seems only PyBytes use this copy_from_slice and it doesn't own PyObjectRef /PyRef
|
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): impl _IOBase{
...
#[pymethod(magic)]
fn exit(instance: PyObjectRef, _args: FuncArgs, vm: &VirtualMachine) -> PyResult<()> {
vm.call_method(&instance, "close", ())?;
let _ = ManuallyDrop::new(instance);
Ok(())
}
...
} Edited: eliding |
Great! you discovered a deep side of the reason! |
The first deref of a PyObjectRef/PyRef to a dropped object is from
|
There is one extra call to |
Turn out to be a false alarm, the bug is still in my gc code, so close this issue |
Oh, thank you for confirming. I thought you watched it without gc. what's happened? |
I set the is_drop flag too early,didn't realize drop might not happen if del resurrect a object, the actual drop might not happen... |
Bug behavior
Added a
is_drop
field in PyInner. and found out bothBufferedReader
&FileIO
gets double drop? Did some search on the code, no clue where misses aclone()
to PyRef or PyObjectRefPossible 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
Test Code
added
is_drop
field toPyInner<T>
in core.rsThis is the
git diff
patch file(Also added abacktrace
crate for debugging in this patch)drop_guard.patch.txt
And here is core code to check double dropping:
The text was updated successfully, but these errors were encountered: