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

offset_from "same allocation" check is confused by int-to-ptr casts #3767

Closed
RalfJung opened this issue Jul 27, 2024 · 3 comments · Fixed by rust-lang/rust#128277
Closed

Comments

@RalfJung
Copy link
Member

This code fails with -Zmiri-disable-stacked-borrows, but interestingly passes with SB:

use std::{mem::ManuallyDrop, slice};

fn extend_fail() {
    let data = Data::allocate(b"hello");
    assert_eq!(data.read(), b"hello");
    let mut buffer = Vec::<u8>::with_capacity(1024);
    buffer.extend(data.read());
    assert_eq!(buffer.len(), 5);
}

fn main() {
    extend_fail();
}

pub struct Data {
    len: usize,
    ptr: usize,
}

impl Data {
    pub fn allocate(data: &[u8]) -> Self {
        let mut data = ManuallyDrop::new(data.to_owned().into_boxed_slice());
        Self {
            len: data.len(),
            ptr: data.as_mut_ptr() as usize,
        }
    }
    pub fn read(&self) -> &[u8] {
        unsafe {
            // SAFETY: fine since we r/w it directly!
            slice::from_raw_parts(self.ptr as *mut u8, self.len)
        }
    }
}

impl Drop for Data {
    fn drop(&mut self) {
        unsafe {
            Vec::from_raw_parts(self.ptr as *mut u8, self.len, self.len);
        }
    }
}

gives

error: Undefined Behavior: `ptr_offset_from_unsigned` called on pointers into different allocations
   --> /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/slice/iter.rs:136:1
    |
136 | / iterator! {struct Iter -> *const T, &'a T, const, {/* no mut */}, as_ref, {
137 | |     fn is_sorted_by<F>(self, mut compare: F) -> bool
138 | |     where
139 | |         Self: Sized,
...   |
143 | |     }
144 | | }}
    | |__^ `ptr_offset_from_unsigned` called on pointers into different allocations
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
    = note: BACKTRACE:
    = note: inside `std::slice::Iter::<'_, u8>::make_slice` at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/slice/iter/macros.rs:57:26: 57:48
    = note: inside `std::slice::Iter::<'_, u8>::as_slice` at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/slice/iter.rs:132:9: 132:26
    = note: inside `<std::vec::Vec<u8> as std::vec::spec_extend::SpecExtend<&u8, std::slice::Iter<'_, u8>>>::spec_extend` at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/alloc/src/vec/spec_extend.rs:54:21: 54:40
    = note: inside `<std::vec::Vec<u8> as std::iter::Extend<&u8>>::extend::<&[u8]>` at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/alloc/src/vec/mod.rs:3251:9: 3251:43
note: inside `extend_fail`
   --> x.rs:7:5
    |
7   |     buffer.extend(data.read());
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^

I think the "same allocation" check in the offset_from intrinsic doesn't work properly when there are wildcard pointers involved.

@ohsayan
Copy link

ohsayan commented Jul 27, 2024

This is actually what caught me off guard... because there are no different allocations (in this context). The len! macro (called in iter I suppose?) seems to do this:

nd => {
    // To get rid of some bounds checks (see `position`), we use ptr_sub instead of
    // offset_from (Tested by `codegen/slice-position-bounds-check`.)
    // SAFETY: by the type invariant pointers are aligned and `start <= end`
    unsafe { end.sub_ptr($self.ptr) }
},

and that's where the error seems to originate from (which leads to the intrinsic ptr_offset_from_unsigned). I actually also noticed this error with SBs as well somewhere...in a block of code closely related to this (with int2ptr casts). I'll try do a repro; it's actually hard to produce a minimal example because in our code there's a lot of FFI so isolating it (in pure Rust) is quite a bit of work.

I'll try and report back if I'm able to repro it.

@RalfJung
Copy link
Member Author

I have a reproducer:

fn main() {
    let arr = [0; 5];
    let begin = arr.as_ptr();
    let end = begin.wrapping_add(arr.len());
    let end_roundtrip = end as usize as *const i32;
    unsafe { end_roundtrip.offset_from(begin) };
}

@RalfJung
Copy link
Member Author

@ohsayan yeah you were right, this is indeed a false positive. Thanks for reporting!

(That said, Tree Borrows also doesn't support int2ptr cast, but what will almost certainly happen is that it accepts too many programs, not that it gives unexpected errors. Still, in the status quo, the combination of permissive provenance and Tree Borrows is not great as Tree Borrows will just allow any access with a pointer created by int2ptr casts, and also ignore those accesses for the purpose of other pointers being unique/read-only.)

rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jul 29, 2024
Rollup merge of rust-lang#128277 - RalfJung:offset_from_wildcard, r=oli-obk

miri: fix offset_from behavior on wildcard pointers

offset_from wouldn't behave correctly when the "end" pointer was a wildcard pointer (result of an int2ptr cast) just at the end of the allocation. Fix that by expressing the "same allocation" check in terms of two `check_ptr_access_signed` instead of something specific to offset_from, which is both more canonical and works better with wildcard pointers.

The second commit just improves diagnostics: I wanted the "pointer is dangling (has no provenance)" message to say how many bytes of memory it expected to see (since if it were 0 bytes, this would actually be legal, so it's good to tell the user that it's not 0 bytes). And then I was annoying that the error looks so different for when you deref a dangling pointer vs an out-of-bounds pointer so I made them more similar.

Fixes rust-lang/miri#3767
github-actions bot pushed a commit that referenced this issue Jul 30, 2024
miri: fix offset_from behavior on wildcard pointers

offset_from wouldn't behave correctly when the "end" pointer was a wildcard pointer (result of an int2ptr cast) just at the end of the allocation. Fix that by expressing the "same allocation" check in terms of two `check_ptr_access_signed` instead of something specific to offset_from, which is both more canonical and works better with wildcard pointers.

The second commit just improves diagnostics: I wanted the "pointer is dangling (has no provenance)" message to say how many bytes of memory it expected to see (since if it were 0 bytes, this would actually be legal, so it's good to tell the user that it's not 0 bytes). And then I was annoying that the error looks so different for when you deref a dangling pointer vs an out-of-bounds pointer so I made them more similar.

Fixes #3767
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

Successfully merging a pull request may close this issue.

2 participants