Undefined behavior in `test::send_patch` due to invalid read

[icmccorm]

I've been experimenting with a version of Miri that can execute foreign functions by interpreting the LLVM bytecode that is produced during a crate's build process. We're hoping our results can assist with the Krabcake project.

Miri found a violation of Tree Borrows in the following test case:

 test::send_patch ... error: Undefined Behavior: deallocation through <93226> is forbidden
   --> .../rust/library/alloc/src/alloc.rs:117:14
    |
117 |     unsafe { __rust_dealloc(ptr, layout.size(), layout.align()) }
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ deallocation through <93226> is forbidden
    |
    = help: the accessed tag <93226> is a child of the conflicting tag <93157>
    = help: the conflicting tag <93157> has state Frozen which forbids this deallocation (acting as a child write access)
help: the accessed tag <93226> was created here
   --> src/lib.rs:573:31
    |
573 |           let t = thread::spawn(move || {
    |  _______________________________^
574 | |             let mut computed_new = String::new();
575 | |             patch.read_to_string(&mut computed_new).unwrap();
576 | |         });
    | |_________^
help: the conflicting tag <93157> was created here, in the initial state Reserved
   --> .../rust/library/std/src/panic.rs:141:55
    |
141 | pub fn catch_unwind<F: FnOnce() -> R + UnwindSafe, R>(f: F) -> Result<R> {
    |                                                       ^
help: the conflicting tag <93157> later transitioned to Frozen due to a reborrow (acting as a foreign read access) at offsets [0x0..0x10]
   --> src/lib.rs:452:9
    |
452 |         (*h).borrow_mut()
    |         ^^^^
    = help: this transition corresponds to a loss of write permissions
    = note: BACKTRACE (of the first span):
    = note: inside `std::alloc::dealloc` at .../rust/library/alloc/src/alloc.rs:117:14: 117:64
    = note: inside `<std::alloc::Global as std::alloc::Allocator>::deallocate` at .../rust/library/alloc/src/alloc.rs:254:22: 254:51
    = note: inside `<std::boxed::Box<std::rc::Rc<std::cell::RefCell<dyn ReadAndSeek>>> as std::ops::Drop>::drop` at .../rust/library/alloc/src/boxed.rs:1235:17: 1235:66
    = note: inside `std::ptr::drop_in_place::<std::boxed::Box<std::rc::Rc<std::cell::RefCell<dyn ReadAndSeek>>>> - shim(Some(std::boxed::Box<std::rc::Rc<std::cell::RefCell<dyn ReadAndSeek>>>))` at .../rust/library/core/src/ptr/mod.rs:497:1: 497:56
    = note: inside `std::ptr::drop_in_place::<Patch<'_, std::io::Cursor<&str>, std::io::BufReader<std::io::Cursor<std::vec::Vec<u8>>>>> - shim(Some(Patch<'_, std::io::Cursor<&str>, std::io::BufReader<std::io::Cursor<std::vec::Vec<u8>>>>))` at .../rust/library/core/src/ptr/mod.rs:497:1: 497:56
    = note: inside `std::ptr::drop_in_place::<{closure@src/lib.rs:573:31: 573:38}> - shim(Some({closure@src/lib.rs:573:31: 573:38}))` at .../rust/library/core/src/ptr/mod.rs:497:1: 497:56
note: inside closure
   --> src/lib.rs:576:9
    |
576 |         });

This appears to be caused by the following reborrow, which occurs on line 350 within the implementation of Patch::with_buf_read:

    let job = unsafe { raw::rs_patch_begin(patch_copy_cb, mem::transmute(&*cb_data)) };

When the pointer created by mem::transmute(&*cb_data) is read through in the callback patch_copy_cb, it invalidates the pointer associated with the Box within the instance of the Patch struct that is moved into the move closure on line 573. At the end of this closure, when Patch is dropped, the Box is deallocated using a pointer that is no longer valid due to this foreign read.

To fix this issue, you could use Box::into_raw to obtain a raw pointer to the underlying allocation. Then, you'd use this pointer as both the second argument to rs_patch_begin and the field raw of the struct Patch. Since this pointer would be "Reserved", it would allow foreign read accesses, which prevents this error from occurring.

I'd make a pull request with this change, but it would require an implementation of Drop for Patch so that you could call Box::from_raw. However, this also prevents the function Patch::into_inner from borrow checking, since you can't move out of a struct with a Drop implementation.

[mbrt]

Thanks for the report!

I honestly have zero time to dedicate to this library, so if there's an easy PR you could contribute to, I would be grateful. librsync-rs is not stable, so it's fine to break the interface with a version bump if that makes things easier.