Skip to content

Conversation

@theemathas
Copy link

Moving a #[may_dangle] type in Drop::drop asserts the validity of references stored in that type, which is unsound. For example, the following code is UB according to Miri:

#![feature(dropck_eyepatch)]
#![allow(unused)]

struct Thing<T>(Option<T>);

unsafe impl<#[may_dangle] T> Drop for Thing<T> {
    fn drop(&mut self) {
        let _ = self.0.take();
    }
}

fn main() {
    let thing;
    {
        let a = 1;
        thing = Thing(Some(&a));
    }
    // thing is dropped here
}

Moving a `#[may_dangle]` type in `Drop::drop` asserts the validity of references stored in that type, which is unsound. For example, the following code is UB according to Miri:

```rust
#![feature(dropck_eyepatch)]
#![allow(unused)]

struct Thing<T>(Option<T>);

unsafe impl<#[may_dangle] T> Drop for Thing<T> {
    fn drop(&mut self) {
        let _ = self.0.take();
    }
}

fn main() {
    let thing;
    {
        let a = 1;
        thing = Thing(Some(&a));
    }
    // thing is dropped here
}
```
The attribute can be applied to any number of lifetime and type parameters. In
the following example, we assert that we access no data behind a reference of
lifetime `'b` and that the only uses of `T` will be moves or drops, but omit
lifetime `'b` and that the only uses of `T` will be drops, but omit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the change here is correct as described in RFC: https://github.com/rust-lang/rfcs/blob/master/text/1327-dropck-param-eyepatch.md#the-eyepatch-attribute

When used on a type, e.g. #[may_dangle] T, the programmer is asserting the only uses of values of that type will be to move or drop them. Thus, no fields will be accessed nor methods called on values of such a type (apart from any access performed by the destructor for the type when the values are dropped). This ensures that no dangling references (such as when T is instantiated with &'a u32) are ever accessed in the scenario where 'a has the same lifetime as the value being currently destroyed (and thus the precise order of destruction between the two is unknown to the compiler).

In your example, the moved value will be dropped by Options::take, not us, while we have to take responsibility of drop here instead. When we use something like ManualDrop, it would be sound.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean. Miri still detects UB in this code:

#![feature(dropck_eyepatch)]
#![allow(unused)]

struct Thing<T>(Option<T>);

unsafe impl<#[may_dangle] T> Drop for Thing<T> {
    fn drop(&mut self) {
        std::mem::forget(self.0.take());
    }
}

fn main() {
    let thing;
    {
        let a = 1;
        thing = Thing(Some(&a));
    }
    // thing is dropped here
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unsound because drop is occured while we have to ensure moved value will not be used (i.e. dropped by destructor). Just move something is still sound.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand which Drop you're talking about. Could you write code that demonstrates this?

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 this pull request may close these issues.

2 participants