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.

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

Jump to bottom

Document subtleties of ManuallyDrop #130279

Merged
merged 4 commits into from
Sep 27, 2024
Merged

Document subtleties of ManuallyDrop #130279

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e2c9831
Document subtleties of `ManuallyDrop`
theemathas Sep 12, 2024
6fa092a
Address WaffleLapkin's comments
theemathas Sep 13, 2024
f604ed6
Fix awkward wording.
theemathas Sep 13, 2024
de2eba1
Reword ManuallyDrop+Box interaction
theemathas Sep 21, 2024
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
129 changes: 117 additions & 12 deletions library/core/src/mem/manually_drop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,21 @@
use crate::ops::{Deref, DerefMut, DerefPure};
use crate::ptr;

/// A wrapper to inhibit the compiler from automatically calling `T`’s destructor.
/// This wrapper is 0-cost.
/// A wrapper to inhibit the compiler from automatically calling `T`’s
/// destructor. This wrapper is 0-cost.
///
/// `ManuallyDrop<T>` is guaranteed to have the same layout and bit validity as
/// `T`, and is subject to the same layout optimizations as `T`. As a consequence,
/// it has *no effect* on the assumptions that the compiler makes about its
/// contents. For example, initializing a `ManuallyDrop<&mut T>` with [`mem::zeroed`]
/// is undefined behavior. If you need to handle uninitialized data, use
/// [`MaybeUninit<T>`] instead.
/// `T`, and is subject to the same layout optimizations as `T`. As a
/// consequence, it has *no effect* on the assumptions that the compiler makes
/// about its contents. For example, initializing a `ManuallyDrop<&mut T>` with
/// [`mem::zeroed`] is undefined behavior. If you need to handle uninitialized
/// data, use [`MaybeUninit<T>`] instead.
///
/// Note that accessing the value inside a `ManuallyDrop<T>` is safe.
/// This means that a `ManuallyDrop<T>` whose content has been dropped must not
/// be exposed through a public safe API.
/// Correspondingly, `ManuallyDrop::drop` is unsafe.
/// Note that accessing the value inside a `ManuallyDrop<T>` is safe. This means
/// that a `ManuallyDrop<T>` whose content has been dropped must not be exposed
/// through a public safe API. Correspondingly, `ManuallyDrop::drop` is unsafe.
///
/// # `ManuallyDrop` and drop order.
/// # `ManuallyDrop` and drop order
///
/// Rust has a well-defined [drop order] of values. To make sure that fields or
/// locals are dropped in a specific order, reorder the declarations such that
Expand All @@ -40,9 +39,115 @@ use crate::ptr;
/// }
/// ```
///
/// # Interaction with `Box`
///
/// Currently, once the `Box<T>` inside a `ManuallyDrop<Box<T>>` is dropped,
/// moving the `ManuallyDrop<Box<T>>` is [considered to be undefined
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 documentation should clarify that this also applies to any types that directly contain Box<_>, like (Box<T>, u8) or struct Foo(Box<T>);.

I also believe that per current rules, calling ManuallyDrop::drop() on any type that directly contains Box<_> is insta-UB, even if you don’t move it afterwards. Technically, it doesn’t neccessarily count as producing an invalid value:

“Producing” a value happens any time a value is assigned to or read from a place, passed to a function/primitive operation or returned from a function/primitive operation.

but at least touching it in any way (like creating a reference) is certainly UB.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't name the place after calling ManuallyDrop::drop, everything is fine; the place isn't touched after that point. Because ManuallyDrop does not have any drop glue, it does not get a drop_in_place terminator at the end of scope, and thus the place is never "used" implicitly by the language, and no reference is created unless you create it yourself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have reworded the text to clarify that the UB also applies to types containing Box.

I am pretty sure that calling ManuallyDrop::drop() on a type containing Box is not UB. rust-lang/unsafe-code-guidelines#245 only states that using the ManuallyDrop later is UB. If you believe that ManuallyDrop::drop() is insta-UB, then I would like a citation to prove otherwise.

Of note, Miri doesn't seem to complain anything if I create a reference to an already-dropped ManuallyDrop<Box<T>>. I believe this is because we are still undecided on whether the existence of a &T implies that the T must be valid.

Copy link
Contributor

Choose a reason for hiding this comment

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

Re-reading a reference, I don’t think I can prove that it’s insta-UB. This is certainly UB per reference though:

let mut x = ManuallyDrop::new(Box::new(42));
unsafe { ManuallyDrop::drop(&mut x); }
let _y = &x;

since it produces (by writing to a place) an invalid value. Reference takes a conservative stance and declares references to invalid values to be invalid:

A reference or Box that is dangling, misaligned, or points to an invalid value (in case of dynamically sized types, using the actual dynamic type of the pointee as determined by the metadata).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From rust-lang/unsafe-code-guidelines#412:

Note that the Rust reference currently answers ["whether a reference requires the pointed-to data to be valid"] with "yes", but in my view this is mostly because we haven't yet figured out what exactly the weaker requirement is that we actually want to impose.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I know. I still think that documentation should be coherent: if we document this as being unsound in reference, it should be also documented as being unsound in other places that deal with the same situation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just found out that the nightly version of the reference now states:

A reference or Box<T> must be aligned, it cannot be dangling, and it must point to a valid value (in case of dynamically sized types, using the actual dynamic type of the pointee as determined by the metadata). Note that the last point (about pointing to a valid value) remains a subject of some debate.

/// behavior](https://github.com/rust-lang/unsafe-code-guidelines/issues/245).
/// That is, the following code causes undefined behavior:
///
/// ```no_run
/// use std::mem::ManuallyDrop;
///
/// let mut x = ManuallyDrop::new(Box::new(42));
/// unsafe {
/// ManuallyDrop::drop(&mut x);
/// }
/// let y = x; // Undefined behavior!
/// ```
///
/// This is [likely to change in the
/// future](https://rust-lang.github.io/rfcs/3336-maybe-dangling.html). In the
/// meantime, consider using [`MaybeUninit`] instead.
///
/// # Safety hazards when storing `ManuallyDrop` in a struct or an enum.
///
/// Special care is needed when all of the conditions below are met:
/// * A struct or enum contains a `ManuallyDrop`.
/// * The `ManuallyDrop` is not inside a `union`.
/// * The struct or enum is part of public API, or is stored in a struct or an
/// enum that is part of public API.
/// * There is code that drops the contents of the `ManuallyDrop` field, and
/// this code is outside the struct or enum's `Drop` implementation.
///
/// In particular, the following hazards may occur:
///
/// #### Storing generic types
///
/// If the `ManuallyDrop` contains a client-supplied generic type, the client
/// might provide a `Box` as that type. This would cause undefined behavior when
/// the struct or enum is later moved, as mentioned in the previous section. For
/// example, the following code causes undefined behavior:
///
/// ```no_run
/// use std::mem::ManuallyDrop;
///
/// pub struct BadOption<T> {
/// // Invariant: Has been dropped iff `is_some` is false.
/// value: ManuallyDrop<T>,
/// is_some: bool,
/// }
/// impl<T> BadOption<T> {
/// pub fn new(value: T) -> Self {
/// Self { value: ManuallyDrop::new(value), is_some: true }
/// }
/// pub fn change_to_none(&mut self) {
/// if self.is_some {
/// self.is_some = false;
/// unsafe {
/// // SAFETY: `value` hasn't been dropped yet, as per the invariant
/// // (This is actually unsound!)
/// ManuallyDrop::drop(&mut self.value);
/// }
/// }
/// }
/// }
///
/// // In another crate:
///
/// let mut option = BadOption::new(Box::new(42));
/// option.change_to_none();
/// let option2 = option; // Undefined behavior!
/// ```
///
/// #### Deriving traits
///
/// Deriving `Debug`, `Clone`, `PartialEq`, `PartialOrd`, `Ord`, or `Hash` on
/// the struct or enum could be unsound, since the derived implementations of
/// these traits would access the `ManuallyDrop` field. For example, the
/// following code causes undefined behavior:
theemathas marked this conversation as resolved.
Show resolved Hide resolved
///
/// ```no_run
/// use std::mem::ManuallyDrop;
///
/// // This derive is unsound in combination with the `ManuallyDrop::drop` call.
/// #[derive(Debug)]
/// pub struct Foo {
/// value: ManuallyDrop<String>,
/// }
/// impl Foo {
/// pub fn new() -> Self {
/// let mut temp = Self {
/// value: ManuallyDrop::new(String::from("Unsafe rust is hard."))
/// };
/// unsafe {
/// // SAFETY: `value` hasn't been dropped yet.
/// ManuallyDrop::drop(&mut temp.value);
/// }
/// temp
/// }
/// }
///
/// // In another crate:
///
/// let foo = Foo::new();
/// println!("{:?}", foo); // Undefined behavior!
/// ```
///
/// [drop order]: https://doc.rust-lang.org/reference/destructors.html
/// [`mem::zeroed`]: crate::mem::zeroed
/// [`MaybeUninit<T>`]: crate::mem::MaybeUninit
/// [`MaybeUninit`]: crate::mem::MaybeUninit
#[stable(feature = "manually_drop", since = "1.20.0")]
#[lang = "manually_drop"]
#[derive(Copy, Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
Expand Down
Loading