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.

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

Clarify the relationship between forget() and ManuallyDrop. #69618

Merged
merged 4 commits into from
Mar 20, 2020
Merged
Changes from all commits
Commits
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
65 changes: 49 additions & 16 deletions src/libcore/mem/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@ pub use crate::intrinsics::transmute;
///
/// # Examples
///
/// Leak an I/O object, never closing the file:
/// The canonical safe use of `mem::forget` is to circumvent a value's destructor
/// implemented by the `Drop` trait. For example, this will leak a `File`, i.e. reclaim
/// the space taken by the variable but never close the underlying system resource:
///
/// ```no_run
/// use std::mem;
Expand All @@ -68,9 +70,40 @@ pub use crate::intrinsics::transmute;
/// mem::forget(file);
/// ```
///
/// The practical use cases for `forget` are rather specialized and mainly come
/// up in unsafe or FFI code. However, [`ManuallyDrop`] is usually preferred
/// for such cases, e.g.:
/// This is useful when the ownership of the underlying resource was previously
/// transferred to code outside of Rust, for example by transmitting the raw
/// file descriptor to C code.
///
/// # Relationship with `ManuallyDrop`
///
/// While `mem::forget` can also be used to transfer *memory* ownership, doing so is error-prone.
/// [`ManuallyDrop`] should be used instead. Consider, for example, this code:
///
/// ```
/// use std::mem;
///
/// let mut v = vec![65, 122];
/// // Build a `String` using the contents of `v`
/// let s = unsafe { String::from_raw_parts(v.as_mut_ptr(), v.len(), v.capacity()) };
/// // leak `v` because its memory is now managed by `s`
/// mem::forget(v); // ERROR - v is invalid and must not be passed to a function
/// assert_eq!(s, "Az");
/// // `s` is implicitly dropped and its memory deallocated.
/// ```
///
/// There are two issues with the above example:
///
/// * If more code were added between the construction of `String` and the invocation of
/// `mem::forget()`, a panic within it would cause a double free because the same memory
hniksic marked this conversation as resolved.
Show resolved Hide resolved
/// is handled by both `v` and `s`.
/// * After calling `v.as_mut_ptr()` and transmitting the ownership of the data to `s`,
/// the `v` value is invalid. Even when a value is just moved to `mem::forget` (which won't
/// inspect it), some types have strict requirements on their values that
/// make them invalid when dangling or no longer owned. Using invalid values in any
/// way, including passing them to or returning them from functions, constitutes
/// undefined behavior and may break the assumptions made by the compiler.
///
/// Switching to `ManuallyDrop` avoids both issues:
///
/// ```
/// use std::mem::ManuallyDrop;
Expand All @@ -80,24 +113,24 @@ pub use crate::intrinsics::transmute;
/// // does not get dropped!
/// let mut v = ManuallyDrop::new(v);
/// // Now disassemble `v`. These operations cannot panic, so there cannot be a leak.
/// let ptr = v.as_mut_ptr();
/// let cap = v.capacity();
/// let (ptr, len, cap) = (v.as_mut_ptr(), v.len(), v.capacity());
/// // Finally, build a `String`.
/// let s = unsafe { String::from_raw_parts(ptr, 2, cap) };
/// let s = unsafe { String::from_raw_parts(ptr, len, cap) };
/// assert_eq!(s, "Az");
/// // `s` is implicitly dropped and its memory deallocated.
/// ```
///
/// Using `ManuallyDrop` here has two advantages:
/// `ManuallyDrop` robustly prevents double-free because we disable `v`'s destructor
/// before doing anything else. `mem::forget()` doesn't allow this because it consumes its
/// argument, forcing us to call it only after extracting anything we need from `v`. Even
/// if a panic were introduced between construction of `ManuallyDrop` and building the
/// string (which cannot happen in the code as shown), it would result in a leak and not a
/// double free. In other words, `ManuallyDrop` errs on the side of leaking instead of
/// erring on the side of (double-)dropping.
///
/// * We do not "touch" `v` after disassembling it. For some types, operations
/// such as passing ownership (to a function like `mem::forget`) requires them to actually
/// be fully owned right now; that is a promise we do not want to make here as we are
/// in the process of transferring ownership to the new `String` we are building.
/// * In case of an unexpected panic, `ManuallyDrop` is not dropped, but if the panic
/// occurs before `mem::forget` was called we might end up dropping invalid data,
/// or double-dropping. In other words, `ManuallyDrop` errs on the side of leaking
/// instead of erring on the side of dropping.
/// Also, `ManuallyDrop` prevents us from having to "touch" `v` after transferring the
/// ownership to `s` - the final step of interacting with `v` to dispoe of it without
/// running its destructor is entirely avoided.
///
/// [drop]: fn.drop.html
/// [uninit]: fn.uninitialized.html
Expand Down