Can Drop::drop invalidate the value? (i.e. can drop_in_place rely on the value staying valid-but-unusable)

[Manishearth]

See: https://github.com/rust-lang/rust/pull/108684 and this zulip discussion

Firstly, let's assume that ManuallyDrop::drop() is equivalent to ptr::drop_in_place() since the docs claim they are.

ManuallyDrop::drop() makes an interesting claim:

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.

This can be parsed in two ways:

(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.

and

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.)

The question becomes: is Drop::drop() allowed to invalidate the type? Is it fine for its body to be *self = uninit or e.g. for a bool something like *self = transmute(73)?

This has different implications in a world with shallow vs deep validity (https://github.com/rust-lang/unsafe-code-guidelines/issues/77), because if there is deep validity this means that it might be insta-UB to pass an &mut T to ptr::drop_in_place() (otoh it might be insta-UB to write a Drop impl that invalidates its own &mut self, so it still could be okay, depending on precisely how long validity is supposed to last)

As it currently stands with the ambiguity, the note of "as far as the compiler is concerned still holds a bit-pattern which is valid for the type T." is not really useful. We should figure out what it means, and clarify the docs to say so.

[sollyucko]

I think the comma between "unchanged" and "and" would be spurious in the second interpretation, so I think the first is more likely. Would whoever added that to the docs be able to give useful input to this discussion?

[JakobDegen]

As it currently stands with the ambiguity, the note of "as far as the compiler is concerned still holds a bit-pattern which is valid for the type T." is not really useful. We should figure out what it means, and clarify the docs to say so.

120% agreement to this. I actually wouldn't mind a PR that removes this phrasing

[Manishearth]

I'd rather have something to that effect left in if we can manage to get alignment on what it should be.

[JakobDegen]

It's worth noting too that this has significance on the ManuallyDrop API - people may expect to be able to move a ManuallyDrop<T> after dropping the T, but if these kinds of drop impls are ok then that's unsound.

I don't know which of these two is more likely to cause issues for users.

[JakobDegen]

I'd rather have something to that effect left in if we can manage to get alignment on what it should be.

Agreed. But until we can agree on what it should be, I'm not so sure that sentence is better than just saying nothing

[Manishearth]

I think we should declare such Drop impls as unsound because:

• I don't think being able to invalidate that particular memory is that useful
• I suspect it's in the wild a little but but only around deep validity; I can imagine pointers geting deep-invalidated in Drop impls but not shallow invalidated. And deep validity is already known to be something that will have cause problems in the wild. In other words, we can make this change without unduly declaring tons of existing code unsound, except in the case where we have made another choice that declares tons of existing code unsound anyway.
• Drop::drop() is called shortly before calling destructors on the various fields, which is a "use" of the fields, so it seems to even fall out of "don't use invalid values". There's definitely some fudging here around Copy types, though.
• It does make it easier to use drop_in_place especially with e.g. &mut T
• The whole thing around moving ManuallyDrops.

Overall it feels like declaring these as unsound is low-risk-high-reward.

I'm not so sure that sentence is better than just saying nothing

I wonder if we can make the sentence say something useful for the specific case (where you know the type's Drop impl) as opposed to the generic one.

[CAD97]

At least somewhat relevant is that align_of_val_raw (and size_) is required to work on a pointer to dropped value. For all current type kinds this is just a function on the pointer metadata making this a vacuously true assertion, but this is the one place I know we actually mention "using" a dropped place.

[RalfJung]

(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.

That is the interpretation I always had in mind.

Given that Option<ManuallyDrop<Box<T>>> is a thing, we have a big problem if the destructor can change the bit pattern to something invalid. That said, I think this is just a safety guarantee, not a validity guarantee -- so "the compiler" in the quote above seems a bit misleading. I don't see a good way via which we could actually have language UB from a drop that puts in a bad value; this is basically https://github.com/rust-lang/unsafe-code-guidelines/issues/84.

[JakobDegen]

Given that Option<ManuallyDrop<Box>> is a thing, we have a big problem if the destructor can change the bit pattern to something invalid

Strictly speaking, I don't think this is true. Dropping the contents of a ManuallyDrop is unsafe, and I don't see any documentation anywhere that requires that moving a ManuallyDrop<T> after the T has been dropped is sound. Does user code rely on this? Probably, so I don't think we should actually break it

[RalfJung]

I would expect that if such a move was wrong that has to be documented in ManuallyDrop::drop. Things are move-by-default in Rust.

[newpavlov]

I don't think being able to invalidate that particular memory is that useful

This is important in the context of zeroizing cryptographic secrets. For example, we would like to define roughly the following function in the zeroize crate (let's forget about lack of exact guarantees for black_box for now):

pub unsafe fn zeroize_flat_type<T: Sized>(data: *mut T) {
    ptr::drop_in_place(data);
    ptr::write_bytes(data, 0, 1);
    hint::black_box(data);
}

#[repr(transparent)]
pub struct ZeroizeOnDrop<T: Sized>(pub T);

impl<T: Sized> Drop for ZeroizeOnDrop<T> {
    fn drop(&mut self) {
        unsafe { zeroize_flat_type(&mut self.0); }
    }
}

And then it would be really nice for ZeroizeOnDrop<NonZeroU32> to be safe.

Also, isn't Vec's Drop impl invalidates itself in a similar way?

[chorman0773]

Note that your two code snippets are mutually recursive.

Also, isn't Vec's Drop impl invalidates itself in a similar way?

It invalidates its own safety invariant but not its validity invariant. Drop is entitled to invalidate the safety invariant no question, because safe code can't call Drop again.

[newpavlov]

Note that your two code snippets are mutually recursive.

Ah, yes. I will fix the example.

[CAD97]

Related: whether ManuallyDrop implies MaybeDangling semantics relies on whether drop_in_place may invalidate the properties which MaybeDangling suppresses. Box obviously invalidates the property that it's an allocated pointer in its drop_in_place; is this a validity invariant, a safety invariant, or some secret third thing?

Zulip discussion: https://rust-lang.zulipchat.com/#narrow/stream/136281-t-opsem/topic/Moving.20.60ManuallyDrop.3CBox.3C_.3E.3E.60