rust-lang / miri

An interpreter for Rust's mid-level intermediate representation
Apache License 2.0
4.37k stars 337 forks source link

Memory-reusing custom allocator fails in miri #3666

Closed FeanorTheElf closed 3 months ago

FeanorTheElf commented 3 months ago

Miri flags the following code of a custom allocator that caches allocation as UB. I think it isn't, although I am not 100% sure.

#![feature(slice_ptr_get)]
#![feature(allocator_api)]
use std::alloc::*;
use std::cell::Cell;
use std::ptr::NonNull;

pub struct ReusingAllocator {
    layout: Layout,
    cached_allocs: Cell<Option<NonNull<[u8]>>>
}

impl ReusingAllocator {

    pub fn new(layout: Layout) -> Self {
        Self {
            layout: layout,
            cached_allocs: Cell::new(None)
        }
    }

    pub fn new_for_slice<T: Sized>(slice_len: usize) -> Self {
        Self::new(Layout::array::<T>(slice_len).unwrap())
    }
}

unsafe impl Allocator for ReusingAllocator {

    fn allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
        if layout != self.layout {
            return Err(AllocError);
        }
        match self.cached_allocs.replace(None) {
            Some(ptr) => Ok(ptr),
            None => Global.allocate(self.layout)
        }
    }

    unsafe fn deallocate(&self, mut ptr: NonNull<u8>, layout: Layout) {
        assert!(layout == self.layout);
        let ptr_as_slice = NonNull::new(std::ptr::slice_from_raw_parts_mut(ptr.as_mut(), self.layout.size())).unwrap();
        match self.cached_allocs.replace(Some(ptr_as_slice)) {
            None => { },
            Some(old_ptr) => { Global.deallocate(NonNull::new(old_ptr.as_mut_ptr()).unwrap(), layout) }
        };
    }
}

impl Drop for ReusingAllocator {

    fn drop(&mut self) {
        match self.cached_allocs.replace(None) {
            None => { },
            Some(old_ptr) => unsafe { Global.deallocate(NonNull::new(old_ptr.as_mut_ptr()).unwrap(), self.layout); }
        }
    }
}

fn main() {
    let allocor = ReusingAllocator::new_for_slice::<u128>(10);

    let mut allocation1 = Vec::with_capacity_in(10, &allocor);
    allocation1.push(0 as u128);
    drop(allocation1);

    let mut allocation2 = Vec::with_capacity_in(10, &allocor);
    allocation2.push(0 as u128)
}

The error message is

error: Undefined Behavior: attempting a write access using <229343> at alloc78817[0x1], but that tag does not exist in the borrow stack for this location
    --> C:\Users\Simon\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\alloc\src\vec\mod.rs:2003:13
     |
2003 |             ptr::write(end, value);
     |             ^^^^^^^^^^^^^^^^^^^^^^
     |             |
     |             attempting a write access using <229343> at alloc78817[0x1], but that tag does not exist in the borrow stack for this location
     |             this error occurs as part of an access at alloc78817[0x0..0x10]
     |
     = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
     = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <229343> was created by a SharedReadWrite retag at offsets [0x0..0x1]
    --> src\testmiri.rs:38:76
     |
38   |         let ptr_as_slice = NonNull::new(std::ptr::slice_from_raw_parts_mut(ptr.as_mut(), self.layout.size())).unwrap();
     |                                                                            ^^^^^^^^^^^^
     = note: BACKTRACE (of the first span) on thread `testmiri::test_reusing_allocator`:
     = note: inside `std::vec::Vec::<u128, &testmiri::ReusingAllocator>::push` at C:\Users\Simon\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\alloc\src\vec\mod.rs:2003:13: 2003:35
note: inside `testmiri::test_reusing_allocator`
    --> src\testmiri.rs:65:5
     |
65   |     allocation2.push(0 as u128)
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside closure
    --> src\testmiri.rs:57:28
     |
56   | #[test]
     | ------- in this procedural macro expansion
57   | fn test_reusing_allocator() {
     |                            ^
     = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)
saethlin commented 3 months ago

This is a copy-and-paste of my reply on this issue from 2 days ago: https://github.com/rust-lang/miri/issues/3657

This is not a false positive. The ranges printed by the diagnostics are exclusive on the end, like Rust ranges. 0..5 does not include 5.

Alternatively if you mean you think that Stacked Borrows should be changed to accept this code, that's part of what Tree Borrows is for. Set MIRIFLAGS=-Zmiri-tree-borrows to use that model instead. Your code is trying to do the &Header pattern: https://github.com/rust-lang/unsafe-code-guidelines/issues/256

I'm closing this because it is not a bug in Miri (which essentially does not have false positives), but I don't want to discourage you from asking further questions about this situation or diagnostic.