Skip to content

Commit

Permalink
Rollup merge of #127446 - zachs18:miri-stdlib-leaks-core-alloc, r=Mar…
Browse files Browse the repository at this point in the history
…k-Simulacrum

Remove memory leaks in doctests in `core`, `alloc`, and `std`

cc `@RalfJung`  #126067 rust-lang/miri#3670

Should be no actual *documentation* changes[^1], all added/modified lines in the doctests are hidden with `#`,

This PR splits the existing memory leaks in doctests in `core`, `alloc`, and `std` into two general categories:

1. "Non-focused" memory leaks that are incidental to the thing being documented, and/or are easy to remove, i.e. they are only there because preventing the leak would make the doctest less clear and/or concise.
    - These doctests simply have a comment like `# // Prevent leaks for Miri.` above the added line that removes the memory leak.
    - [^2]Some of these would perhaps be better as part of the public documentation part of the doctest, to clarify that a memory leak can happen if it is not otherwise mentioned explicitly in the documentation  (specifically the ones in `(A)Rc::increment_strong_count(_in)`).
2. "Focused" memory leaks that are intentional and documented, and/or are possibly fragile to remove.
    - These doctests have a `# // FIXME` comment above the line that removes the memory leak, with a note that once `-Zmiri-disable-leak-check` can be applied at test granularity, these tests should be "un-unleakified" and have `-Zmiri-disable-leak-check` enabled.
    - Some of these are possibly fragile (e.g. unleaking the result of `Vec::leak`) and thus should definitely not be made part of the documentation.

This should be all of the leaks currently in `core` and `alloc`. I only found one leak in `std`, and it was in the first category (excluding the modules `@RalfJung` mentioned in #126067 , and reducing the number of iterations of [one test](https://github.com/rust-lang/rust/blob/master/library/std/src/sync/once_lock.rs#L49-L94) from 1000 to 10)

[^1]: assuming [^2] is not added
[^2]: backlink
  • Loading branch information
workingjubilee committed Jul 14, 2024
2 parents 5c56577 + 51727ba commit 285d45d
Show file tree
Hide file tree
Showing 11 changed files with 59 additions and 5 deletions.
6 changes: 6 additions & 0 deletions library/alloc/src/boxed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1213,6 +1213,9 @@ impl<T: ?Sized, A: Allocator> Box<T, A> {
/// let static_ref: &'static mut usize = Box::leak(x);
/// *static_ref += 1;
/// assert_eq!(*static_ref, 42);
/// # // FIXME(https://github.com/rust-lang/miri/issues/3670):
/// # // use -Zmiri-disable-leak-check instead of unleaking in tests meant to leak.
/// # drop(unsafe { Box::from_raw(static_ref) });
/// ```
///
/// Unsized data:
Expand All @@ -1222,6 +1225,9 @@ impl<T: ?Sized, A: Allocator> Box<T, A> {
/// let static_ref = Box::leak(x);
/// static_ref[0] = 4;
/// assert_eq!(*static_ref, [4, 2, 3]);
/// # // FIXME(https://github.com/rust-lang/miri/issues/3670):
/// # // use -Zmiri-disable-leak-check instead of unleaking in tests meant to leak.
/// # drop(unsafe { Box::from_raw(static_ref) });
/// ```
#[stable(feature = "box_leak", since = "1.26.0")]
#[inline]
Expand Down
6 changes: 6 additions & 0 deletions library/alloc/src/rc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1277,6 +1277,8 @@ impl<T: ?Sized> Rc<T> {
///
/// let five = Rc::from_raw(ptr);
/// assert_eq!(2, Rc::strong_count(&five));
/// # // Prevent leaks for Miri.
/// # Rc::decrement_strong_count(ptr);
/// }
/// ```
#[inline]
Expand Down Expand Up @@ -1345,6 +1347,8 @@ impl<T: ?Sized, A: Allocator> Rc<T, A> {
/// let x = Rc::new("hello".to_owned());
/// let x_ptr = Rc::into_raw(x);
/// assert_eq!(unsafe { &*x_ptr }, "hello");
/// # // Prevent leaks for Miri.
/// # drop(unsafe { Rc::from_raw(x_ptr) });
/// ```
#[must_use = "losing the pointer will leak memory"]
#[stable(feature = "rc_raw", since = "1.17.0")]
Expand Down Expand Up @@ -1572,6 +1576,8 @@ impl<T: ?Sized, A: Allocator> Rc<T, A> {
///
/// let five = Rc::from_raw_in(ptr, System);
/// assert_eq!(2, Rc::strong_count(&five));
/// # // Prevent leaks for Miri.
/// # Rc::decrement_strong_count_in(ptr, System);
/// }
/// ```
#[inline]
Expand Down
3 changes: 3 additions & 0 deletions library/alloc/src/string.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1984,6 +1984,9 @@ impl String {
/// let x = String::from("bucket");
/// let static_ref: &'static mut str = x.leak();
/// assert_eq!(static_ref, "bucket");
/// # // FIXME(https://github.com/rust-lang/miri/issues/3670):
/// # // use -Zmiri-disable-leak-check instead of unleaking in tests meant to leak.
/// # drop(unsafe { Box::from_raw(static_ref) });
/// ```
#[stable(feature = "string_leak", since = "1.72.0")]
#[inline]
Expand Down
6 changes: 6 additions & 0 deletions library/alloc/src/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1414,6 +1414,8 @@ impl<T: ?Sized> Arc<T> {
/// // the `Arc` between threads.
/// let five = Arc::from_raw(ptr);
/// assert_eq!(2, Arc::strong_count(&five));
/// # // Prevent leaks for Miri.
/// # Arc::decrement_strong_count(ptr);
/// }
/// ```
#[inline]
Expand Down Expand Up @@ -1484,6 +1486,8 @@ impl<T: ?Sized, A: Allocator> Arc<T, A> {
/// let x = Arc::new("hello".to_owned());
/// let x_ptr = Arc::into_raw(x);
/// assert_eq!(unsafe { &*x_ptr }, "hello");
/// # // Prevent leaks for Miri.
/// # drop(unsafe { Arc::from_raw(x_ptr) });
/// ```
#[must_use = "losing the pointer will leak memory"]
#[stable(feature = "rc_raw", since = "1.17.0")]
Expand Down Expand Up @@ -1766,6 +1770,8 @@ impl<T: ?Sized, A: Allocator> Arc<T, A> {
/// // the `Arc` between threads.
/// let five = Arc::from_raw_in(ptr, System);
/// assert_eq!(2, Arc::strong_count(&five));
/// # // Prevent leaks for Miri.
/// # Arc::decrement_strong_count_in(ptr, System);
/// }
/// ```
#[inline]
Expand Down
7 changes: 6 additions & 1 deletion library/alloc/src/vec/into_iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,10 +120,15 @@ impl<T, A: Allocator> IntoIter<T, A> {
/// This is roughly equivalent to the following, but more efficient
///
/// ```
/// # let mut into_iter = Vec::<u8>::with_capacity(10).into_iter();
/// # let mut vec = Vec::<u8>::with_capacity(10);
/// # let ptr = vec.as_mut_ptr();
/// # let mut into_iter = vec.into_iter();
/// let mut into_iter = std::mem::replace(&mut into_iter, Vec::new().into_iter());
/// (&mut into_iter).for_each(drop);
/// std::mem::forget(into_iter);
/// # // FIXME(https://github.com/rust-lang/miri/issues/3670):
/// # // use -Zmiri-disable-leak-check instead of unleaking in tests meant to leak.
/// # drop(unsafe { Vec::<u8>::from_raw_parts(ptr, 0, 10) });
/// ```
///
/// This method is used by in-place iteration, refer to the vec::in_place_collect
Expand Down
6 changes: 6 additions & 0 deletions library/alloc/src/vec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1473,6 +1473,9 @@ impl<T, A: Allocator> Vec<T, A> {
/// // 2. `0 <= capacity` always holds whatever `capacity` is.
/// unsafe {
/// vec.set_len(0);
/// # // FIXME(https://github.com/rust-lang/miri/issues/3670):
/// # // use -Zmiri-disable-leak-check instead of unleaking in tests meant to leak.
/// # vec.set_len(3);
/// }
/// ```
///
Expand Down Expand Up @@ -2391,6 +2394,9 @@ impl<T, A: Allocator> Vec<T, A> {
/// let static_ref: &'static mut [usize] = x.leak();
/// static_ref[0] += 1;
/// assert_eq!(static_ref, &[2, 2, 3]);
/// # // FIXME(https://github.com/rust-lang/miri/issues/3670):
/// # // use -Zmiri-disable-leak-check instead of unleaking in tests meant to leak.
/// # drop(unsafe { Box::from_raw(static_ref) });
/// ```
#[stable(feature = "vec_leak", since = "1.47.0")]
#[inline]
Expand Down
3 changes: 3 additions & 0 deletions library/core/src/mem/manually_drop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ impl<T> ManuallyDrop<T> {
/// x.truncate(5); // You can still safely operate on the value
/// assert_eq!(*x, "Hello");
/// // But `Drop` will not be run here
/// # // FIXME(https://github.com/rust-lang/miri/issues/3670):
/// # // use -Zmiri-disable-leak-check instead of unleaking in tests meant to leak.
/// # let _ = ManuallyDrop::into_inner(x);
/// ```
#[must_use = "if you don't need the wrapper, you can use `mem::forget` instead"]
#[stable(feature = "manually_drop", since = "1.20.0")]
Expand Down
15 changes: 15 additions & 0 deletions library/core/src/mem/maybe_uninit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,8 @@ impl<T> MaybeUninit<T> {
/// use std::mem::MaybeUninit;
///
/// let v: MaybeUninit<Vec<u8>> = MaybeUninit::new(vec![42]);
/// # // Prevent leaks for Miri
/// # unsafe { let _ = MaybeUninit::assume_init(v); }
/// ```
///
/// [`assume_init`]: MaybeUninit::assume_init
Expand Down Expand Up @@ -446,6 +448,9 @@ impl<T> MaybeUninit<T> {
/// let mut x = MaybeUninit::<String>::uninit();
///
/// x.write("Hello".to_string());
/// # // FIXME(https://github.com/rust-lang/miri/issues/3670):
/// # // use -Zmiri-disable-leak-check instead of unleaking in tests meant to leak.
/// # unsafe { MaybeUninit::assume_init_drop(&mut x); }
/// // This leaks the contained string:
/// x.write("hello".to_string());
/// // x is initialized now:
Expand Down Expand Up @@ -506,6 +511,8 @@ impl<T> MaybeUninit<T> {
/// // Create a reference into the `MaybeUninit<T>`. This is okay because we initialized it.
/// let x_vec = unsafe { &*x.as_ptr() };
/// assert_eq!(x_vec.len(), 3);
/// # // Prevent leaks for Miri
/// # unsafe { MaybeUninit::assume_init_drop(&mut x); }
/// ```
///
/// *Incorrect* usage of this method:
Expand Down Expand Up @@ -545,6 +552,8 @@ impl<T> MaybeUninit<T> {
/// let x_vec = unsafe { &mut *x.as_mut_ptr() };
/// x_vec.push(3);
/// assert_eq!(x_vec.len(), 4);
/// # // Prevent leaks for Miri
/// # unsafe { MaybeUninit::assume_init_drop(&mut x); }
/// ```
///
/// *Incorrect* usage of this method:
Expand Down Expand Up @@ -746,6 +755,8 @@ impl<T> MaybeUninit<T> {
/// use std::mem::MaybeUninit;
///
/// let mut x = MaybeUninit::<Vec<u32>>::uninit();
/// # let mut x_mu = x;
/// # let mut x = &mut x_mu;
/// // Initialize `x`:
/// x.write(vec![1, 2, 3]);
/// // Now that our `MaybeUninit<_>` is known to be initialized, it is okay to
Expand All @@ -755,6 +766,8 @@ impl<T> MaybeUninit<T> {
/// x.assume_init_ref()
/// };
/// assert_eq!(x, &vec![1, 2, 3]);
/// # // Prevent leaks for Miri
/// # unsafe { MaybeUninit::assume_init_drop(&mut x_mu); }
/// ```
///
/// ### *Incorrect* usages of this method:
Expand Down Expand Up @@ -1088,6 +1101,8 @@ impl<T> MaybeUninit<T> {
/// let init = MaybeUninit::clone_from_slice(&mut dst, &src);
///
/// assert_eq!(init, src);
/// # // Prevent leaks for Miri
/// # unsafe { std::ptr::drop_in_place(init); }
/// ```
///
/// ```
Expand Down
2 changes: 2 additions & 0 deletions library/core/src/ptr/non_null.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1663,6 +1663,8 @@ impl<T> NonNull<[T]> {
/// // Note that calling `memory.as_mut()` is not allowed here as the content may be uninitialized.
/// # #[allow(unused_variables)]
/// let slice: &mut [MaybeUninit<u8>] = unsafe { memory.as_uninit_slice_mut() };
/// # // Prevent leaks for Miri.
/// # unsafe { Global.deallocate(memory.cast(), Layout::new::<[u8; 32]>()); }
/// # Ok::<_, std::alloc::AllocError>(())
/// ```
#[inline]
Expand Down
3 changes: 3 additions & 0 deletions library/std/src/sync/condvar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ impl WaitTimeoutResult {
/// let pair = Arc::new((Mutex::new(false), Condvar::new()));
/// let pair2 = Arc::clone(&pair);
///
/// # let handle =
/// thread::spawn(move || {
/// let (lock, cvar) = &*pair2;
///
Expand All @@ -58,6 +59,8 @@ impl WaitTimeoutResult {
/// break
/// }
/// }
/// # // Prevent leaks for Miri.
/// # let _ = handle.join();
/// ```
#[must_use]
#[stable(feature = "wait_timeout", since = "1.5.0")]
Expand Down
7 changes: 3 additions & 4 deletions src/bootstrap/mk/Makefile.in
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,8 @@ check-aux:
library/core \
library/alloc \
--no-doc
# Some doctests have intentional memory leaks.
# Some use file system operations to demonstrate dealing with `Result`.
$(Q)MIRIFLAGS="-Zmiri-ignore-leaks -Zmiri-disable-isolation" \
# Some doctests use file system operations to demonstrate dealing with `Result`.
$(Q)MIRIFLAGS="-Zmiri-disable-isolation" \
$(BOOTSTRAP) miri --stage 2 \
library/core \
library/alloc \
Expand All @@ -70,7 +69,7 @@ check-aux:
$(BOOTSTRAP) miri --stage 2 library/std \
--no-doc -- \
--skip fs:: --skip net:: --skip process:: --skip sys::pal::
$(Q)MIRIFLAGS="-Zmiri-ignore-leaks -Zmiri-disable-isolation" \
$(Q)MIRIFLAGS="-Zmiri-disable-isolation" \
$(BOOTSTRAP) miri --stage 2 library/std \
--doc -- \
--skip fs:: --skip net:: --skip process:: --skip sys::pal::
Expand Down

0 comments on commit 285d45d

Please sign in to comment.