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

Allocators that "shrink" allocations to fit the request cause Stacked Borrows errors when used with Box #2104

Open
RalfJung opened this issue May 7, 2022 · 6 comments
Labels
A-aliasing Area: This affects the aliasing model (Stacked/Tree Borrows) A-allocator Area: related to memory allocation C-bug Category: This is a bug.

Comments

@RalfJung
Copy link
Member

RalfJung commented May 7, 2022

When a Box allocator creates an allocation and returns only a part of that to Box, there will be Stacked Borrows errors when the Box is deallocated. This is because the pointer lost the provenance for the other parts of memory outside the Box, and hence does not have the right to deallocate them.

This affects, in particular, the System allocator on Windows:

#![feature(allocator_api)]

#[repr(align(32))]
struct VeryAligned(u8);

fn main() {
    let _ = Box::new_in(VeryAligned(0), std::alloc::System);
}

Also see the discussion on Zulip.

@RalfJung RalfJung added C-bug Category: This is a bug. A-aliasing Area: This affects the aliasing model (Stacked/Tree Borrows) labels Jun 5, 2022
@Nemo157
Copy link
Member

Nemo157 commented Aug 18, 2022

It's not just shrinking, allocators that overallocate (as allowed by the Allocator::allocate docs) also fail under miri:

#![feature(allocator_api)]

use core::ptr::NonNull;
use std::alloc::{Allocator, Layout, AllocError, System};

struct OverAllocate;

unsafe impl Allocator for OverAllocate {
    fn allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
        let (layout, _) = layout.extend(layout).unwrap();
        let layout = layout.pad_to_align();
        System.allocate(layout)
    }
    unsafe fn deallocate(&self, ptr: NonNull<u8>, layout: Layout) {
        let (layout, _) = layout.extend(layout).unwrap();
        let layout = layout.pad_to_align();
        System.deallocate(ptr, layout);
    }
}

fn main() {
    let _ = Box::new_in(0, OverAllocate);
}
error: Undefined Behavior: no item granting write access for deallocation to tag <3690> at alloc1837 found in borrow stack
  --> /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/unix/alloc.rs:42:9
   |
42 |         libc::free(ptr as *mut libc::c_void)
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no item granting write access for deallocation to tag <3690> at alloc1837 found in borrow stack
   |
   = 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: <3690> was created by a retag at offsets [0x0..0x4]
  --> src/main.rs:22:13
   |
22 |     let _ = Box::new_in(0, OverAllocate);
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: backtrace:
   = note: inside `std::sys::unix::alloc::<impl std::alloc::GlobalAlloc for std::alloc::System>::dealloc` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/unix/alloc.rs:42:9
   = note: inside `<std::alloc::System as std::alloc::Allocator>::deallocate` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/alloc.rs:216:22
note: inside `<OverAllocate as std::alloc::Allocator>::deallocate` at src/main.rs:17:9
  --> src/main.rs:17:9
   |
17 |         System.deallocate(ptr, layout);
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: inside `alloc::alloc::box_free::<i32, OverAllocate>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:348:9
   = note: inside `std::ptr::drop_in_place::<std::boxed::Box<i32, OverAllocate>> -

(If you assume this check in miri is correct, then fixing Box is not enough; if you combine this with Box::from_raw(Box::leak(...)) it seems to me that the Allocator::allocate and Box::leak docs are in direct contradiction under the SB rules).

@jtroo

This comment was marked as off-topic.

@saethlin

This comment was marked as off-topic.

@RalfJung
Copy link
Member Author

It's not just shrinking, allocators that overallocate (as allowed by the Allocator::allocate docs) also fail under miri:

That is what I mean by shrinking: allocating more, and then only returning a part of that from the allocation request (hence "shrinking" the underlying allocation to fit the request).


We now have the very experimental Tree Borrows mode in Miri, which should be able to handle code like this. You can enable it via -Zmiri-tree-borrows.

@Nemo157
Copy link
Member

Nemo157 commented May 22, 2023

By "overallocating" I mean allocating something larger than the requested layout and returning the entire larger allocation, not returning a subset.

@RalfJung
Copy link
Member Author

RalfJung commented May 22, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-aliasing Area: This affects the aliasing model (Stacked/Tree Borrows) A-allocator Area: related to memory allocation C-bug Category: This is a bug.
Projects
None yet
Development

No branches or pull requests

4 participants