Skip to content
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

Memory-reusing custom allocator fails in miri #3666

Closed
FeanorTheElf opened this issue Jun 10, 2024 · 1 comment
Closed

Memory-reusing custom allocator fails in miri #3666

FeanorTheElf opened this issue Jun 10, 2024 · 1 comment

Comments

@FeanorTheElf
Copy link

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
Copy link
Member

This is a copy-and-paste of my reply on this issue from 2 days ago: #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: rust-lang/unsafe-code-guidelines#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants