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

Confusing Stacked Borrows error with #![feature(ptr_metadata)] #2087

Closed
dimpolo opened this issue Apr 27, 2022 · 5 comments
Closed

Confusing Stacked Borrows error with #![feature(ptr_metadata)] #2087

dimpolo opened this issue Apr 27, 2022 · 5 comments

Comments

@dimpolo
Copy link

dimpolo commented Apr 27, 2022

Sorry for the vague title.
I wasn't sure if this code was valid so I ran it through Miri.
This however led to even more confusion.
It seems as though I can convince Miri that this code is fine, by first calling a function with no observable side-effect.

I would be very thankful for an explanation (and even more thankful if someone want's to tell me if this code is fine 😃)

#![feature(ptr_metadata)]

use std::fmt::Debug;
use std::ptr;
use std::ptr::DynMetadata;

#[repr(C)]
struct Container<T: ?Sized> {
    vtable: DynMetadata<dyn Debug>,
    value: T,
}

impl<T: Debug + 'static> Container<T> {
    const fn new(value: T) -> Self {
        Container {
            vtable: ptr::metadata(ptr::null::<T>() as *const dyn Debug),
            value,
        }
    }

    const fn erase(&self) -> &Container<()> {
        let ptr = self as *const Container<T> as *const Container<()>;

        unsafe { &*ptr }
    }

    fn make_fat(&self) -> &Container<dyn Debug> {
        let fat_pointer = ptr::from_raw_parts(self as *const _ as *const (), self.vtable);
        unsafe { &*fat_pointer }
    }
}

fn main() {
    static CONTAINER: Container<i32> = Container::new(123);
    static ERASED: &Container<()> = CONTAINER.erase();

    // CONTAINER.make_fat(); uncomment this line to make Miri happy?

    ERASED.make_fat();
}
@bjorn3
Copy link
Member

bjorn3 commented Apr 27, 2022

The erase function produces a reference which is only valid for an object of the size size_of::<Container<()>>() which would be a single pointer in this case. make_fat however wants to produce a reference valid for an object of size size_of::<Container<i32>>() which would be two pointers in this case. As for why CONTAINER.make_fat() fixes the issue I think it is because of -Zmiri-tag-raw-pointers not being enabled by default. Passing it will likely cause your code to error even with CONTAINER.make_fat(). See rust-lang/unsafe-code-guidelines#134 for more discussion on the problem you are facing.

@dimpolo
Copy link
Author

dimpolo commented Apr 27, 2022

That's intersting.
So Miri is able to deduce inside make_fat that the T used to be i32? Does it look inside the vtable to do so?
Your are right about -Zmiri-tag-raw-pointers, with the flag it errors in both cases.

Thanks for the response and the links, it helped clear things up. If I stay in raw pointer land and not create the ERASED intermediate reference, things seem to work out

@bjorn3
Copy link
Member

bjorn3 commented Apr 28, 2022

So Miri is able to deduce inside make_fat that the T used to be i32?

When entering make_fat in case of CONTAINER.make_fat() you still have &Container<i32> which is valid for the entire length of Container<i32>. When entering make_fat in case of ERASED.make_fat() you only got a reference valid for the length of Container<()>.

As indicated in rust-lang/unsafe-code-guidelines#134 stacked borrows may in the future be relaxed to allow your code. At the very least I'm not aware of the current lowering to LLVM ir having UB for this.

@dimpolo
Copy link
Author

dimpolo commented Apr 28, 2022

This mostly makes sense to me. I guess my last question might be more about the implementation details of Miri.

When entering make_fat in case of ERASED.make_fat() you only got a reference valid for the length of Container<()>.

I understand that when using &Container<dyn Debug> this would require a reference valid for the length of Container<i32>. What surprises me is that, at time of creation the validity of &Container<dyn Debug> is already too short.

I understand why that would make sense, but not how Miri manages to come to that conclusion, since it seems as though at that point the type information of i32 is not present.

Anyways, thanks for clearing things up. Confusion has been (mostly) lifted.

@dimpolo dimpolo closed this as completed Apr 28, 2022
@RalfJung
Copy link
Member

So Miri is able to deduce inside make_fat that the T used to be i32? Does it look inside the vtable to do so?

Yes, Miri does the equivalent of size_of_val do determine the memory range that the reference is valid for.

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