Skip to content

Clarify drop_in_place safety #108684

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+7 −2
Open

Clarify drop_in_place safety #108684

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
23d4d77
Clarify drop_in_place safety
Manishearth Mar 3, 2023
f6747b0
Remove note on references
Manishearth Mar 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions library/core/src/ptr/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,10 @@ mod mut_ptr;
/// done automatically by the compiler. This means the fields of packed structs
/// are not dropped in-place.
///
/// [`drop_in_place()`] does not modify the pointed-to value beyond any changes
/// performed by [`Drop::drop()`]. As far as the compiler is concerned, the value
Copy link
Member

Choose a reason for hiding this comment

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

What is the point of this guarantee? drop can still mutate the value pretty much arbitrarily, right?
Or is it intended to say something about the auto-generated drop glue, in case I control the drop being called? That seems odd to state like this though, instead we should give a somewhat operational description of the auto-generated drop glue:

  • for a struct, it will first call the user-defined drop if it exists, and then call the drop glue of each field
  • for an enum, it will first call the user-defined drop if it exists, and then determine the active variant and call the drop glue for all fields of that variant
  • for an array, it will call the drop glue for each field
  • ...

Also IMO we shouldn't duplicate the same thing between ManuallyDrop::drop and here. drop_in_place is probably the more canonical location, but then ManuallyDrop::drop should just reference this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or is it intended to say something about the auto-generated drop glue, in case I control the drop being called?

Yes.

/// will still contain a valid bit pattern for type `T`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// will still contain a valid bit pattern for type `T`.
/// must still contain a valid bit pattern for type `T`.

I feel like "will" is a little odd here, this reads better to me -- but in general I find the sentence strange; typically we don't I think say things like "As far as the compiler ...", rather that something is UB or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

You'd say that except that party of the wording was lifted straight from the MaybeUninit docs :)

As Jakob says it doesn't make much sense though, so we should probably tighten up the wording on both

Copy link
Member

Choose a reason for hiding this comment

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

You mean the ManuallyDrop docs, I assume. But yeah that wording doesn't make a ton of sense. I don't know what that sentence is even trying to achieve, can we just remove it?

///
/// [`ptr::read`]: self::read
/// [`ptr::read_unaligned`]: self::read_unaligned
/// [pinned]: crate::pin
Expand All @@ -446,10 +450,15 @@ mod mut_ptr;
/// additional invariants - this is type-dependent.
///
/// Additionally, if `T` is not [`Copy`], using the pointed-to value after
/// calling `drop_in_place` can cause undefined behavior. Note that `*to_drop =
/// calling `drop_in_place` may cause undefined behavior. Note that `*to_drop =
/// foo` counts as a use because it will cause the value to be dropped
/// again. [`write()`] can be used to overwrite data without causing it to be
/// dropped.
/// dropped. Read operations may be UB based on library invariants of that type,
/// for example reading the value pointed to by a dropped `Box<T>` is a use-after-free.
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence seems useful, but can you clarify: Read operations on what? Ie, I have code like:

let p: *mut T = some_pointer_from_somewhere;
drop_in_place(p);

I initially interpreted read operations as being about *p, but that doesn't seem to agree with the rest of the sentence, which seems to be about **p in the case where T is Box<S>.

///
/// Having an `&` or `&mut` reference to the pointed-to value after calling [`drop_in_place()`]
/// is still sound as long as it is not read from (in which case the soundness of the operation
/// depends on the specific type).
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not so sure that we should say this. drop_in_place does not on its own deinitialize, but Drop::drop code might, and it's undecided if holding a & to uninit data is sound.

Copy link
Member Author

Choose a reason for hiding this comment

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

Drop::drop() also has an &mut reference, if Drop::drop() is allowed to deinit it then you are allowed to hold on to an &mut reference after drop_in_place. I do not think there is any situation where Drop::drop() may soundly invalidate the type in a world where holding &-to-uninit is unsound.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there definitely is. The issue is that "holding" is a really imprecise term. Specifically, consider:

struct S(u8);

impl Drop for S {
    fn drop(&mut self) {
        let p = self as *mut S as *mut MaybeUninit<u8>;
        *p = MaybeUninit::uninit();
        // In the world I'm talking about, this would be UB, if it was actually written:
        // let r = self;
    }
}

let mut s = S;
let r = &mut s;
drop_in_place(r);
// But the move about was not actually included, and so no UB. However, we include it here:
let r_immut = &*r; // or let r2 = r, either is possible
// Now we have UB

This world isn't particularly likely, but it's definitely not unimaginable

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be nice to just get a ruling on this because as it stands this leads to the API being super weird and annoying.

Note that ManuallyDrop already has this guarantee; I'm just importing it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually! The guarantee on ManuallyDrop prevents the above drop impl from being considered safe, because such a drop impl would make ManuallyDrop::drop() leave the type in an uninit state.

Copy link
Member

Choose a reason for hiding this comment

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

Note that ManuallyDrop already has this guarantee;

The statement about & or &mut seems stronger than anything ManuallyDrop says, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

From the docs of ManuallyDrop::drop():

This is exactly equivalent to calling ptr::drop_in_place with a pointer to the contained value

..

Other than changes made by the destructor itself, the memory is left unchanged, and so as far as the compiler is concerned still holds a bit-pattern which is valid for the type T.

Perhaps there's ambiguity on how the "other than changes made by the destructor itself" groups with the rest of the statement, but to me this is saying that one can rely on ManuallyDrop::drop() leaving T in a situation of being valid, so drop_in_place must as well.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, ok, so it's an open question whether &T validity depends on valid T -- but either way, if we're supposing that drop does leave a valid T, then &T remains valid too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah; it's an open question but I think it's not actually needed to be answered to reach my conclusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've opened rust-lang/unsafe-code-guidelines#394 and will change the docs to use the same squirrely wording that ManuallyDrop uses

///
/// Note that even if `T` has size `0`, the pointer must be non-null and properly aligned.
///
Expand Down