rust-lang / miri

An interpreter for Rust's mid-level intermediate representation
Apache License 2.0
4.64k stars 349 forks source link

Possibly false-positive UB in std::ptr::from_raw_parts for DST Types #4058

Closed hieronymusma closed 1 day ago

hieronymusma commented 1 day ago

Hello,

first and foremost: I'm not actually sure if this is a false-positive, intended behavior, or a Rust compiler bug. But I couldn't find any information otherwise, which is why I am opening an issue here.

Code example:

#![feature(ptr_metadata)]
#![feature(pointer_is_aligned_to)]

#[cfg(test)]
mod tests {
    #[repr(C)]
    struct DST {
        data: u32,
        dst: [u8],
    }

    struct AlignedData {
        _align: [u32; 0],
        data: [u8; 6],
    }

    #[test]
    fn wrong_dst_size() {
        let ptr = {
            static DATA: AlignedData = AlignedData {
                _align: [],
                // First 4 bytes for a single u32 value
                // Then 2 bytes for the dst
                data: [1, 2, 3, 4, 1, 2],
            };
            DATA.data.as_ptr()
        };
        assert!(ptr.is_aligned_to(4));
        let _dst = unsafe { &*std::ptr::from_raw_parts::<DST>(ptr, 2) };
    }
}

In this example, a reference to a dynamically sized type is created by using the std::ptr::from_raw_parts function to create a fat-pointer.

Miri is complaining in the following way when executing cargo miri test:

test tests::wrong_dst_size ... error: Undefined Behavior: trying to retag from <263939> for SharedReadOnly permission at alloc12[0x6], but that tag does not exist in the borrow stack for this location
  --> src/lib.rs:29:29
   |
29 |         let _dst = unsafe { &*std::ptr::from_raw_parts::<DST>(ptr, 2) };
   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |                             |
   |                             trying to retag from <263939> for SharedReadOnly permission at alloc12[0x6], but that tag does not exist in the borrow stack for this location
   |                             this error occurs as part of retag at alloc12[0x0..0x8]

If I ignore the alignment and directly specify the data as an &[u8], I can get an even preciser error message:

test tests::wrong_dst_size ... error: Undefined Behavior: out-of-bounds pointer use: expected a pointer to 8 bytes of memory, but got alloc13 which is only 6 bytes from the end of the allocation
  --> src/lib.rs:31:29
   |
31 |         let _dst = unsafe { &*std::ptr::from_raw_parts::<DST>(ptr, 2) };
   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds pointer use: expected a pointer to 8 bytes of memory, but got alloc13 which is only 6 bytes from the end of the allocation

In my understanding, the size of the DST should be 6 bytes and not 8 bytes, right?

When reading about the type layout of repr(C) on https://doc.rust-lang.org/reference/type-layout.html#the-c-representation it states "Finally, the size of the struct is the current offset rounded up to the nearest multiple of the struct’s alignment.".

I guess this is exactly what happens here, but is that the right behavior?

Just to give you an idea why I need this: I'm writing a device tree parser for a device tree which is put by QEMU in memory. I've defined the device tree similar to the DST struct in the example above (first fixed size header fields and then dynamically sized data at the end). However, the size of the data in memory is not a multiple of 4 which is why Miri complains about the error I got above.

I know I could solve that differently, however I feel like a DST Type is very neat and perfect for that use case.

In case this is really a bug, I would be glad to help provide a fix. But first, I want to clarify if this is a real bug or just intended behavior and I'm missing something.

Thank you for your time in advance!

bjorn3 commented 1 day ago

DST is 4 byte aligned due to the u32 field. The size of a type is always a whole multiple of the alignment, so DST would be 4, 8, 12, ... bytes. Never 6 bytes as that is not a whole multiple of 4.

ChrisDenton commented 1 day ago

"Finally, the size of the struct is the current offset rounded up to the nearest multiple of the struct’s alignment.".

I guess this is exactly what happens here, but is that the right behavior?

If it's the documented behaviour then it is right that miri checks it. If you want to change documented behaviour then you'll need to do so on the rust repository. But that would need a lot of discussion first, which usually happens on https://rust-lang.zulipchat.com/

Though tbh I'm sceptical that this would be changed.

hieronymusma commented 1 day ago

Alright, thank you for the clarification!