From e2c983138876cc29d89c9320b05a6614607d11c7 Mon Sep 17 00:00:00 2001 From: "Tim (Theemathas) Chirananthavat" Date: Thu, 12 Sep 2024 23:39:45 +0700 Subject: [PATCH 1/4] Document subtleties of `ManuallyDrop` --- library/core/src/mem/manually_drop.rs | 126 +++++++++++++++++++++++--- 1 file changed, 115 insertions(+), 11 deletions(-) diff --git a/library/core/src/mem/manually_drop.rs b/library/core/src/mem/manually_drop.rs index be5cee2e85267..f4b56c9f13e73 100644 --- a/library/core/src/mem/manually_drop.rs +++ b/library/core/src/mem/manually_drop.rs @@ -1,20 +1,19 @@ 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` 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`] 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`] instead. /// -/// Note that accessing the value inside a `ManuallyDrop` is safe. -/// This means that a `ManuallyDrop` 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` is safe. This means +/// that a `ManuallyDrop` whose content has been dropped must not be exposed +/// through a public safe API. Correspondingly, `ManuallyDrop::drop` is unsafe. /// /// # `ManuallyDrop` and drop order. /// @@ -40,9 +39,114 @@ use crate::ptr; /// } /// ``` /// +/// # Interaction with `Box`. +/// +/// Currently, once the `Box` inside a `ManuallyDrop>` is dropped, +/// moving the `ManuallyDrop>` is [considered to be undefined +/// 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 may change in the future. In the meantime, consider using +/// [`MaybeUninit`] instead. +/// +/// # Safety hazards when storing `ManuallyDrop` in a struct / enum. +/// +/// Special care is needed when all of the conditions below are met: +/// * A field of a struct or enum is a `ManuallyDrop` or contains a +/// `ManuallyDrop`, without the `ManuallyDrop` being inside a `union`. +/// * The struct or enum is part of public API, or is stored in a struct or enum +/// that is part of public API. +/// * There is code outside of a `Drop` implementation that calls +/// [`ManuallyDrop::drop`] or [`ManuallyDrop::take`] on the `ManuallyDrop` +/// field. +/// +/// In particular, the following hazards can occur: +/// +/// #### Storing generic types +/// +/// If the `ManuallyDrop` contains a client-supplied generic type, the client +/// might provide a `Box`, causing undefined behavior when the struct / enum is +/// later moved, as mentioned above. For example, the following code causes +/// undefined behavior: +/// +/// ```no_run +/// use std::mem::ManuallyDrop; +/// +/// pub struct BadOption { +/// // Invariant: Has been dropped iff `is_some` is false. +/// value: ManuallyDrop, +/// is_some: bool, +/// } +/// impl BadOption { +/// 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 / enum could be unsound, since the derived implementations of +/// these traits would access the `ManuallyDrop` field. For example, the +/// following code causes undefined behavior: +/// +/// ```no_run +/// use std::mem::ManuallyDrop; +/// +/// #[derive(Debug)] // This is unsound! +/// pub struct Foo { +/// value: ManuallyDrop, +/// } +/// 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`]: 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)] From 6fa092a1024d6fc47926bb8eed9e2df075c2ff2f Mon Sep 17 00:00:00 2001 From: "Tim (Theemathas) Chirananthavat" Date: Fri, 13 Sep 2024 21:26:39 +0700 Subject: [PATCH 2/4] Address WaffleLapkin's comments --- library/core/src/mem/manually_drop.rs | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/library/core/src/mem/manually_drop.rs b/library/core/src/mem/manually_drop.rs index f4b56c9f13e73..24d95f939000a 100644 --- a/library/core/src/mem/manually_drop.rs +++ b/library/core/src/mem/manually_drop.rs @@ -15,7 +15,7 @@ use crate::ptr; /// that a `ManuallyDrop` 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 @@ -39,7 +39,7 @@ use crate::ptr; /// } /// ``` /// -/// # Interaction with `Box`. +/// # Interaction with `Box` /// /// Currently, once the `Box` inside a `ManuallyDrop>` is dropped, /// moving the `ManuallyDrop>` is [considered to be undefined @@ -56,16 +56,17 @@ use crate::ptr; /// let y = x; // Undefined behavior! /// ``` /// -/// This may change in the future. In the meantime, consider using -/// [`MaybeUninit`] instead. +/// 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 / enum. +/// # Safety hazards when storing `ManuallyDrop` in a struct or an enum. /// /// Special care is needed when all of the conditions below are met: /// * A field of a struct or enum is a `ManuallyDrop` or contains a /// `ManuallyDrop`, without the `ManuallyDrop` being inside a `union`. -/// * The struct or enum is part of public API, or is stored in a struct or enum -/// that is part of public API. +/// * 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 outside of a `Drop` implementation that calls /// [`ManuallyDrop::drop`] or [`ManuallyDrop::take`] on the `ManuallyDrop` /// field. @@ -120,14 +121,15 @@ use crate::ptr; /// ```no_run /// use std::mem::ManuallyDrop; /// -/// #[derive(Debug)] // This is unsound! +/// // This derive is unsound in combination with the `ManuallyDrop::drop` call. +/// #[derive(Debug)] /// pub struct Foo { /// value: ManuallyDrop, /// } /// impl Foo { /// pub fn new() -> Self { /// let mut temp = Self { -/// value: ManuallyDrop::new(String::from("Unsafe rust is hard")) +/// value: ManuallyDrop::new(String::from("Unsafe rust is hard.")) /// }; /// unsafe { /// // SAFETY: `value` hasn't been dropped yet. From f604ed631baa3428fcc8370402985a56fc0fc459 Mon Sep 17 00:00:00 2001 From: "Tim (Theemathas) Chirananthavat" Date: Fri, 13 Sep 2024 22:02:07 +0700 Subject: [PATCH 3/4] Fix awkward wording. --- library/core/src/mem/manually_drop.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/library/core/src/mem/manually_drop.rs b/library/core/src/mem/manually_drop.rs index 24d95f939000a..8dea3539e4639 100644 --- a/library/core/src/mem/manually_drop.rs +++ b/library/core/src/mem/manually_drop.rs @@ -63,22 +63,21 @@ use crate::ptr; /// # Safety hazards when storing `ManuallyDrop` in a struct or an enum. /// /// Special care is needed when all of the conditions below are met: -/// * A field of a struct or enum is a `ManuallyDrop` or contains a -/// `ManuallyDrop`, without the `ManuallyDrop` being inside a `union`. +/// * 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 outside of a `Drop` implementation that calls -/// [`ManuallyDrop::drop`] or [`ManuallyDrop::take`] on the `ManuallyDrop` -/// field. +/// * 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 can occur: +/// 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`, causing undefined behavior when the struct / enum is -/// later moved, as mentioned above. For example, the following code causes -/// undefined behavior: +/// 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; @@ -114,7 +113,7 @@ use crate::ptr; /// #### Deriving traits /// /// Deriving `Debug`, `Clone`, `PartialEq`, `PartialOrd`, `Ord`, or `Hash` on -/// the struct / enum could be unsound, since the derived implementations of +/// 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: /// From de2eba18263855871522cb967bfa82ac1ef22d5e Mon Sep 17 00:00:00 2001 From: "Tim (Theemathas) Chirananthavat" Date: Sat, 21 Sep 2024 12:38:38 +0700 Subject: [PATCH 4/4] Reword ManuallyDrop+Box interaction --- library/core/src/mem/manually_drop.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/library/core/src/mem/manually_drop.rs b/library/core/src/mem/manually_drop.rs index 8dea3539e4639..86eac7724cf3a 100644 --- a/library/core/src/mem/manually_drop.rs +++ b/library/core/src/mem/manually_drop.rs @@ -41,8 +41,9 @@ use crate::ptr; /// /// # Interaction with `Box` /// -/// Currently, once the `Box` inside a `ManuallyDrop>` is dropped, -/// moving the `ManuallyDrop>` is [considered to be undefined +/// Currently, if you have a `ManuallyDrop`, where the type `T` is a `Box` or +/// contains a `Box` inside, then dropping the `T` followed by moving the +/// `ManuallyDrop` is [considered to be undefined /// behavior](https://github.com/rust-lang/unsafe-code-guidelines/issues/245). /// That is, the following code causes undefined behavior: ///