katyo / aubio-rs

Rust bindings for Aubio library
36 stars 6 forks source link

Tree borrows violation in `FMat<FMatVecs>` #26

Closed icmccorm closed 6 months ago

icmccorm commented 7 months ago

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.

Miri found an instance of undefined behavior in aubio-rs at version 0.2.1 when running the test filterbank::test::test_filterbank.

error: Borrowing Violation: deallocation through <113586> is forbidden
   --> .../rust/library/alloc/src/alloc.rs:117:14
    |
117 |     unsafe { __rust_dealloc(ptr, layout.size(), layout.align()) }
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ deallocation through <113586> is forbidden
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental
    = help: the accessed tag <113586> is a child of the conflicting tag <113523>
    = help: the conflicting tag <113523> has state Frozen which forbids this deallocation (acting as a child write access)
help: the accessed tag <113586> was created here
   --> src/filterbank.rs:65:5
    |
65  |     }
    |     ^
help: the conflicting tag <113523> was created here, in the initial state Reserved
   --> src/filterbank.rs:55:34
    |
55  |     pub fn set_coeffs(&mut self, filters: FMat<FMatVecs>) {
    |                                  ^^^^^^^
    = help: the conflicting tag <113523> later transitioned to Frozen due to a foreign read access at offsets [0x0..0x8]
    = 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: ...
    = note: inside `std::ptr::drop_in_place::<vec::FMat<'_, std::vec::Vec<*const f32>>> - shim(Some(vec::FMat<'_, std::vec::Vec<*const f32>>))` at .../rust/library/core/src/ptr/mod.rs:497:1: 497:56
note: inside `filterbank::FilterBank::set_coeffs`
   --> src/filterbank.rs:65:5
    |
65  |     }
    |     ^
note: inside `filterbank::test::test_filterbank_do`
   --> src/filterbank.rs:143:9
    |
143 |         filter_bank.set_coeffs(filters.into());
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside closure
   --> src/filterbank.rs:136:29
    |
135 |     #[test]
    |     ------- in this procedural macro expansion
136 |     fn test_filterbank_do() {
    |                             

I believe this is because FMat is a self-referential structure. Consider the implementation of From<T> on line 427 of src/vec.rs:

Self {
    fmat: ffi::fmat_t {
        height: data.len() as _,
        length: data[0].len() as _,
        data: array.as_ptr() as _,
    },
    _x: array,
    _pd: PhantomData,
}

When self.fmat.data is read, it counts as a “foreign read access” under Tree Borrows, which makes the field self._x have a read only permission. This means that when self._x is dropped, it counts as writing with a read-only permission, which is undefined behavior.

To fix this, you would need to modify FMat to only retain a single reference to array instead of having both array and data as aliases.

katyo commented 6 months ago

@icmccorm Could you create PR with fix?

icmccorm commented 6 months ago

My apologies—I think my interpretation above was incorrect. Under the latest version of Tree Borrows (which fixes an issue with Frozen and Reserved permissions) there is no longer an error for this test case. I'll follow up with a pull request if I find anything else but for now I'll close this issue.