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

false positive with SB+TB: "Shifting" of a value #3774

Closed
ohsayan opened this issue Jul 30, 2024 · 6 comments
Closed

false positive with SB+TB: "Shifting" of a value #3774

ohsayan opened this issue Jul 30, 2024 · 6 comments

Comments

@ohsayan
Copy link

ohsayan commented Jul 30, 2024

Consider the following code:

Edit: Small reproducer

fn main() {
    let mut allocations = vec![];
    // allocate
    let alloc = b"hello".to_vec().into_boxed_slice();
    let (ptr, len) = (alloc.as_ptr(), alloc.len());
    allocations.push(alloc);
    // deref
    assert_eq!(
        unsafe {
            // UNSAFE(@ohsayan): allocation still in scope. ptr still valid
            std::slice::from_raw_parts(ptr, len)
        },
        b"hello"
    );
}

Edit: Original code is below

use std::{ptr::NonNull, slice};
fn main() {
    let mut allocator = Allocator::new();
    let fptr = allocator.allocate_new(b"hello");
    assert_eq!(
        unsafe {
            // UNSAFE(@ohsayan): allocator still in scope. ptr still valid
            fptr.deref()
        },
        b"hello"
    );
}

pub struct FatPointer {
    ptr: NonNull<u8>,
    len: usize,
}

impl FatPointer {
    fn new(ptr: NonNull<u8>, len: usize) -> Self {
        Self { ptr, len }
    }
    pub unsafe fn deref(&self) -> &[u8] {
        slice::from_raw_parts(self.ptr.as_ptr(), self.len)
    }
}

pub struct Allocator {
    allocated: Vec<Box<[u8]>>,
}

impl Allocator {
    pub fn new() -> Self {
        Self { allocated: vec![] }
    }
    pub fn allocate_new(&mut self, data: &[u8]) -> FatPointer {
        let data = data.to_vec().into_boxed_slice();
        let ptr = data.as_ptr();
        let l = data.len();
        let fatptr = FatPointer::new(unsafe { NonNull::new_unchecked(ptr as _) }, l);
        self.allocated.push(data);
        return fatptr;
    }
}

Running cargo miri run with both tree and stacked borrows throws the following error;

error: Undefined Behavior: trying to retag from <2720> for SharedReadOnly permission at alloc1143[0x0], but that tag does not exist in the borrow stack for this location
   --> /home/sayan/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/raw.rs:138:9
    |
138 | ...   &*ptr::slice_from_raw_parts(data, len)
    |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |       |
    |       trying to retag from <2720> for SharedReadOnly permission at alloc1143[0x0], but that tag does not exist in the borrow stack for this location
    |       this error occurs as part of retag at alloc1143[0x0..0x5]
    |
    = 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: <2720> was created by a SharedReadOnly retag at offsets [0x0..0x5]
   --> src/main.rs:38:19
    |
38  |         let ptr = data.as_ptr();
    |                   ^^^^^^^^^^^^^
help: <2720> was later invalidated at offsets [0x0..0x5] by a Unique retag
   --> src/main.rs:41:29
    |
41  |         self.allocated.push(data);
    |                             ^^^^
    = note: BACKTRACE (of the first span):
    = note: inside `std::slice::from_raw_parts::<'_, u8>` at /home/sayan/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/raw.rs:138:9: 138:47
note: inside `FatPointer::deref`
   --> src/main.rs:24:9
    |
24  | ...   slice::from_raw_parts(self.ptr.as_ptr(), self.len)
    |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `main`
   --> src/main.rs:8:13
    |
8   |             fptr.deref()
    |             ^^^^^^^^^^^^

I'd have not expected to see this being reported as a TB/SB violation.

@ohsayan
Copy link
Author

ohsayan commented Jul 30, 2024

Ultra small reproducer:

fn main() {
    let mut allocations = vec![];
    // allocate
    let alloc = b"hello".to_vec().into_boxed_slice();
    let (ptr, len) = (alloc.as_ptr(), alloc.len());
    allocations.push(alloc);
    // deref
    assert_eq!(
        unsafe {
            // UNSAFE(@ohsayan): allocation still in scope. ptr still valid
            std::slice::from_raw_parts(ptr, len)
        },
        b"hello"
    );
}

Throws this:

error: Undefined Behavior: trying to retag from <2711> for SharedReadOnly permission at alloc1140[0x0], but that tag does not exist in the borrow stack for this location
   --> /home/sayan/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/raw.rs:138:9
    |
138 | ...   &*ptr::slice_from_raw_parts(data, len)
    |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |       |
    |       trying to retag from <2711> for SharedReadOnly permission at alloc1140[0x0], but that tag does not exist in the borrow stack for this location
    |       this error occurs as part of retag at alloc1140[0x0..0x5]
    |
    = 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: <2711> was created by a SharedReadOnly retag at offsets [0x0..0x5]
   --> src/main.rs:5:23
    |
5   | ..., len) = (alloc.as_ptr(), alloc.len...
    |              ^^^^^^^^^^^^^^
help: <2711> was later invalidated at offsets [0x0..0x5] by a Unique retag
   --> src/main.rs:6:22
    |
6   |     allocations.push(alloc);
    |                      ^^^^^
    = note: BACKTRACE (of the first span):
    = note: inside `std::slice::from_raw_parts::<'_, u8>` at /home/sayan/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/raw.rs:138:9: 138:47
note: inside `main`
   --> src/main.rs:11:13
    |
11  | ...   std::slice::from_raw_parts(ptr, len)
    |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@ohsayan
Copy link
Author

ohsayan commented Jul 30, 2024

Even smaller reproducer

It appears that you just need to pass ownership to trigger this error.

fn main() {
    let mut _blob = None;
    // allocate
    let alloc = Vec::from("hello").into_boxed_slice();
    let (ptr, len) = (alloc.as_ptr(), alloc.len());
    _blob = Some(alloc);
    // deref
    assert_eq!(
        unsafe {
            // UNSAFE(@ohsayan): allocator still in scope. ptr still valid
            std::slice::from_raw_parts(ptr, len)
        },
        b"hello"
    );
}

Edit 1: Further simplify
Edit 2: Fix typo

@RalfJung
Copy link
Member

RalfJung commented Jul 30, 2024

Thanks for the report!

This is currently expected behavior: moving a Box asserts unique ownership of that Box and therefore, all aliasing pointers become invalid and using them again is UB. Box works basically like &mut in that regard. This isn't just a Miri concern; rustc will tell LLVM that Box pointers do not have aliases. Also see the docs here.

rust-lang/unsafe-code-guidelines#326 is the issue where we are tracking whether the rules should be changed (which would require compiler changes). But Miri does correctly implement the rules as they are today.

@ohsayan
Copy link
Author

ohsayan commented Jul 30, 2024

@RalfJung thanks for the explainer. However, I think that this is a pretty significant detail that is easy to miss and should be highlighted in some noticeable place in the docs.

Also, I think in the future relaxing these rules might be a good idea (I checked rust-lang/unsafe-code-guidelines#326) because right now we have to basically make a custom box impl for all of this (instead of a simple to_vec().into_boxed_slice(), for example)

@saethlin
Copy link
Member

The docs call this out here: https://doc.rust-lang.org/stable/std/boxed/index.html#considerations-for-unsafe-code

@RalfJung
Copy link
Member

Once rust-lang/rust#118166 is implemented, using MaybeDangling<Box<T>> will be a pretty good way to relax the rules locally.

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

3 participants