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

MaybeUninit in TakeOwnCell #3

Closed
noamtashma opened this issue Jun 25, 2024 · 1 comment
Closed

MaybeUninit in TakeOwnCell #3

noamtashma opened this issue Jun 25, 2024 · 1 comment

Comments

@noamtashma
Copy link

noamtashma commented Jun 25, 2024

Since TakeOwnCell doesn't put its data inside a MaybeUninit, this means that that data always has to satisfy the validity requirements of that type.

But since that value can be moved away, it might not.

For example, if we create a TakeOwnCell<Box<u32>>, take the box, and then deallocate the box, the data inside the empty TakeOwnCell would hold a pointer to freed memory, which makes it invalid.

I tried to confirm this with miri, but my miri tests passed. I'm not sure why - Maybe I missed something, or maybe this causes the violation to not be found because it's not marked dereferencable because of the UnsafeCell (But I also tried with an older version of miri and it also passed).

However If I didn't misunderstand anything this is still UB.

This can be fixed by putting the data inside a MaybeUninit, since moved away values are basically the same as uninitialized values.

@WaffleLapkin
Copy link
Owner

ManuallyDrop was specifically designed for being able to drop things early, so I think my code is correct :)

Box is not marked with dereferenceable for a similar reason -- dereferenceable is too strong, and would lead to UB in a case like that:

fn f(b: Box<u8>) {
    drop(*b); // move out of the box
    // `b` is not dereferenceable anymore!
}

Box would need a less powerful version like dereferenceable_on_entry (similarly to C++ references lol (which are currently unsound on clang, because it does add dereferenceable)).

Anyway, another way to see that my code is correct is to read the safety contract for ManuallyDrop::take. Note that it only says that the value should not be used afterwards, nothing close to ManuallyDrop::<Box<_>>::take being insta-UB.

ManuallyDrop needs to suppress any validity invariants, which can be changed without touching the value itself (i.e. NonZero validity invariants are fine, but not Box'es, if it must be a valid pointer to be valid), otherwise would be a bug in rust.

Hope that helps <3

@WaffleLapkin WaffleLapkin closed this as not planned Won't fix, can't repro, duplicate, stale Aug 12, 2024
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

2 participants