Tree-borrows false positive with `iter::Extend`?

[ohsayan]

Violating code

While this code might look unnecessarily funky, it is so because I stripped it down to the (hopefully) bare minimum for a repro. Here's the code:

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

#[test]
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);
}

#[test]
fn extend_pass() {
    let data = Data::allocate(b"hello");
    assert_eq!(data.read(), b"hello");
    let mut buffer = Vec::<u8>::with_capacity(1024);
    for byte in data.read() {
        buffer.push(*byte);
    }
    assert_eq!(buffer.len(), 5);
}

pub struct Data {
    data: DataUnion,
}

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

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

struct FatPointer {
    len: u64,
    ptr: usize,
}

impl FatPointer {
    fn new(ptr: usize, len: u64) -> Self {
        Self { len, ptr }
    }
}

union DataUnion {
    x: ManuallyDrop<FatPointer>,
}

What seemed interesting to me is that neither stacked borrows nor tree borrows reports an error with the extend_pass test when run with either of:

MIRIFLAGS="-Zmiri-permissive-provenance -Zmiri-tree-borrows" cargo miri test extend_pass
MIRIFLAGS="-Zmiri-permissive-provenance" cargo miri test extend_pass

But when you try it for extend_fail with tree-borrows, it fails.

Violation

   --> /home/sayan/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/iter.rs:136:1
    |
136 | / iterator! {struct Iter -> *const T, &'a...
137 | |     fn is_sorted_by<F>(self, mut compar...
138 | |     where
139 | |         Self: Sized,
...   |
143 | |     }
144 | | }}
    | |__^ `ptr_offset_from_unsigned` called on pointers into different allocations

Meta

Tried with:

• miri 0.1.0 (8bfcae7 2024-07-23)
• miri 0.1.0 (7120fda 2024-07-25)

[ohsayan]

Update

Looks like the union is unnecessary; I initially added it because it was used in our original code. The below reproduces the exact same error with tree borrows:

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

#[test]
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);
}

#[test]
fn extend_pass() {
    let data = Data::allocate(b"hello");
    assert_eq!(data.read(), b"hello");
    let mut buffer = Vec::<u8>::with_capacity(1024);
    for byte in data.read() {
        buffer.push(*byte);
    }
    assert_eq!(buffer.len(), 5);
}

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);
        }
    }
}

Edit; Further simplified

[ohsayan]

The error seems to originate from this: https://github.com/rust-lang/rust/blob/72d73cec61aa8f85901358cd5d386d5dd066fe52/library/core/src/slice/iter.rs#L136

Also, maybe it's got to do with this: https://github.com/rust-lang/rust/blob/72d73cec61aa8f85901358cd5d386d5dd066fe52/library/core/src/slice/iter/macros.rs#L49? That's the only place where ptr_offset_from_unsigned is used in this context, as far as I can tell.

[RalfJung]

Can you please show the error you are getting? And what makes you think it is a false positive?

[RalfJung]

Running this in Miri, I see warning: integer-to-pointer cast. Tree Borrows doesn't support integer-to-pointer casts, so it is very possible that you will see strange behavior in that combination. We should probably communicate this better.

[RalfJung]

FWIW if you remove the int2ptr casts and just preserve the pointer, it works in Tree Borrows just fine:

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: *mut u8,
}

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(),
        }
    }
    pub fn read(&self) -> &[u8] {
        unsafe {
            // SAFETY: fine since we r/w it directly!
            slice::from_raw_parts(self.ptr, self.len)
        }
    }
}

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

That said, the error you are getting is quite strange. called on pointers into different allocations should not be able to arise from Tree Borrows not supporting int2ptr casts... so there is indeed something wrong here, I opened a new issue for that: #3767. Thanks for pointing this out!