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

Defuse the bomb that is mem::uninitialized #87032

Closed
wants to merge 1 commit into from
Closed

Defuse the bomb that is mem::uninitialized #87032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions library/core/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@
#![feature(slice_ptr_get)]
#![feature(no_niche)] // rust-lang/rust#68303
#![feature(no_coverage)] // rust-lang/rust#84605
#![feature(cfg_sanitize)]
#![deny(unsafe_op_in_unsafe_fn)]
#![deny(or_patterns_back_compat)]

Expand Down
45 changes: 37 additions & 8 deletions library/core/src/mem/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,12 +627,22 @@ pub unsafe fn zeroed<T>() -> T {
}
}

/// Bypasses Rust's normal memory-initialization checks by pretending to
/// produce a value of type `T`, while doing nothing at all.
///
/// **This function is deprecated.** Use [`MaybeUninit<T>`] instead.
///
/// The reason for deprecation is that the function basically cannot be used
/// **This function is deprecated with extreme prejudice.**
/// It is replaced by [`MaybeUninit<T>`], which must be used instead of this function.
Copy link
Member

Choose a reason for hiding this comment

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

Have you looked at how this renders in rustdoc? The standard is one summary sentence, trying to keep it succinct so it renders on, in this case, the std::mem page. The previous one was already a teeny bit long.

///
/// This function produces a value of type `T` that may be of any imaginable
/// bit representation in memory.
/// The exact value that is produced by this function is subject to no
/// guarantees whatsoever, and cannot be relied upon.
/// The value returned by this function may differ between implementations,
/// between different versions of the same implementation,
/// between different compilations of the exact same code with the exact same compiler,
/// between different executions of the same program,
/// between different invocations within a single program,
/// between different *uses* of the same returned value,
/// or for any other reason whatsoever, with no warning.
///
/// The reason this function is deprecated is that it basically cannot be used
/// correctly: it has the same effect as [`MaybeUninit::uninit().assume_init()`][uninit].
/// As the [`assume_init` documentation][assume_init] explains,
/// [the Rust compiler assumes][inv] that values are properly initialized.
Expand All @@ -645,21 +655,40 @@ pub unsafe fn zeroed<T>() -> T {
/// (Notice that the rules around uninitialized integers are not finalized yet, but
/// until they are, it is advisable to avoid them.)
bstrie marked this conversation as resolved.
Show resolved Hide resolved
///
/// Note: because of the aforementioned lack of guarantee about the values returned
/// by this function, and because of the aforementioned inherent unsafety of this API,
/// it is legal for an implementation of Rust to return initialized memory from
/// this function if it so chooses, as a last-ditch safety net.
/// To emphatically reiterate, neither this behavior nor the exact value of any
/// initialized memory may be relied upon.
/// If truly uninitialized memory is desired, then `MaybeUninit` must be used.
///
/// [uninit]: MaybeUninit::uninit
/// [assume_init]: MaybeUninit::assume_init
/// [inv]: MaybeUninit#initialization-invariant
#[inline(always)]
#[rustc_deprecated(since = "1.39.0", reason = "use `mem::MaybeUninit` instead")]
#[stable(feature = "rust1", since = "1.0.0")]
#[allow(deprecated_in_future)]
#[allow(deprecated)]
#[rustc_diagnostic_item = "mem_uninitialized"]
pub unsafe fn uninitialized<T>() -> T {
// Under sanitizers we actually return uninitialized memory,
// as they should be able to check for improper use.
#[cfg(any(miri, sanitize = "memory"))]
// SAFETY: the caller must guarantee that an unitialized value is valid for `T`.
unsafe {
intrinsics::assert_uninit_valid::<T>();
Copy link
Member

Choose a reason for hiding this comment

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

Can you leave this call in place? That will allow us to keep the "attempted to leave type T uninitialized, which is invalid" panics.

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 was under the impression from your previous comments that you were content with only panicking on can't-be-zero types. If your concern is just the diagnostic regression, I would much rather solve this some other way than reintroducing this check, since one of the points of this PR is to deliberately make it possible to use this function with types like bool, so as to "un-break" code that was broken when these checks were added in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

I think I may have misunderstood what was happening.

These panics were intentionally added in #66059, as a way of catching misuses of uninitialized memory. Even though std::mem::uninitialized now 'happens' to zero out these types, users should not be relying on this behavior (as your doc comment states). That is, writing std::mem::uninitialized::<bool>() is still a bug - any code doing so is broken, and should not be 'un-broken' by removing the legitimate panic.

Copy link
Member

Choose a reason for hiding this comment

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

@Aaron1011 I thought the whole point was that mem::uninitialized is broken for any type. Why are types with niches being singled out?

Copy link
Member

Choose a reason for hiding this comment

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

See also @thomcc's comments about how the panics have broken several of his old crates; I would love for this to fix them. https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/disarming.20mem.3A.3Auninitialized/near/244037069

Copy link
Contributor Author

@bstrie bstrie Jul 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It is in fact not at all broken for types that are allowed to be uninitialized, such as MaybeUninit.

Are there any types that are allowed to be uninitialized other than MaybeUninit (and arrays and tuples of MaybeUninit)?

Copy link
Member

Choose a reason for hiding this comment

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

This was started in #79296 but then stalled when the rather large crater results had to be analyzed.

I queued a newer Crater run in #87041, but it will be a while before it gets to the head of the queue and finished.

Copy link
Member

@RalfJung RalfJung Jul 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Are there any types that are allowed to be uninitialized other than MaybeUninit (and arrays and tuples of MaybeUninit)?

Yes:

  • The type () (or really any inhabited zero-sized type... if you only consider the validity invariant, not the safety invariant)
  • Any user-defined union type... well to be fair this is not ratified yet, that's just what I would propose. But I think there is consensus that any union with a field of size zero (essentially, a user defining their own MaybeUninit) is okay to be left uninitialized.

Now, I am not saying that this is all that useful, I just want to get our facts straight so we have a solid basis to make decisions on. :) The only legitimate reason I can think of to call mem::uninitialized is to create an array of MaybeUninit, and we should probably just offer a better API for that (possibly using const generics, like other stdlib APIs are already doing AFAIK).

Copy link
Contributor Author

@bstrie bstrie Jul 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Worth noting here too that even with this PR removing the panic and changing the implementation to zeroed, rustc still independently produces a scary warning when attempting to use uninitialized with any type with invalid values:

warning: the type `bool` does not permit being left uninitialized
 --> un.rs:3:22
  |
3 |     let x = unsafe { std::mem::uninitialized::<bool>() };
  |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |                      |
  |                      this code causes undefined behavior when executed
  |                      help: use `MaybeUninit<T>` instead, and only call `assume_init` after initialization is done
  |
  = note: `#[warn(invalid_value)]` on by default
  = note: booleans must be either `true` or `false`

So even though the panic no longer exists (to the benefit of people using libraries that do this), people writing this code themselves (who will therefore be seeing warnings) will still be made aware that this isn't a good idea (even if, for the moment, it may not actually be invoking UB from the compiler's perspective).

Copy link
Member

Choose a reason for hiding this comment

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

That lint however has a lot of false negatives. For example, it will not fire when the code that calls mem::uninitialized is generic.

MaybeUninit::uninit().assume_init()
}

// When not being sanitized we paper over the problems with this API by returning
// initialized memory, so that legacy code isn't broken
// and has an actual chance to be safe.
#[cfg(not(any(miri, sanitize = "memory")))]
// SAFETY: Because an uninitialized value does not guarantee any specific bit
// representation, it is therefore no less safe to return a zeroed value.
unsafe {
zeroed::<T>()
}
}

/// Swaps the values at two mutable locations, without deinitializing either one.
Expand Down
24 changes: 5 additions & 19 deletions src/test/ui/intrinsics/panic-uninitialized-zeroed.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ fn main() {
// Types that do not like zero-initialziation
test_panic_msg(
|| mem::uninitialized::<fn()>(),
"attempted to leave type `fn()` uninitialized, which is invalid"
"attempted to zero-initialize type `fn()`, which is invalid"
);
test_panic_msg(
|| mem::zeroed::<fn()>(),
Expand All @@ -114,7 +114,7 @@ fn main() {

test_panic_msg(
|| mem::uninitialized::<*const dyn Send>(),
"attempted to leave type `*const dyn std::marker::Send` uninitialized, which is invalid"
"attempted to zero-initialize type `*const dyn std::marker::Send`, which is invalid"
);
test_panic_msg(
|| mem::zeroed::<*const dyn Send>(),
Expand Down Expand Up @@ -145,7 +145,7 @@ fn main() {

test_panic_msg(
|| mem::uninitialized::<(NonNull<u32>, u32, u32)>(),
"attempted to leave type `(std::ptr::NonNull<u32>, u32, u32)` uninitialized, \
"attempted to zero-initialize type `(std::ptr::NonNull<u32>, u32, u32)`, \
which is invalid"
);
test_panic_msg(
Expand All @@ -156,7 +156,7 @@ fn main() {

test_panic_msg(
|| mem::uninitialized::<OneVariant_NonZero>(),
"attempted to leave type `OneVariant_NonZero` uninitialized, \
"attempted to zero-initialize type `OneVariant_NonZero`, \
which is invalid"
);
test_panic_msg(
Expand All @@ -167,7 +167,7 @@ fn main() {

test_panic_msg(
|| mem::uninitialized::<NoNullVariant>(),
"attempted to leave type `NoNullVariant` uninitialized, \
"attempted to zero-initialize type `NoNullVariant`, \
which is invalid"
);
test_panic_msg(
Expand All @@ -176,20 +176,6 @@ fn main() {
which is invalid"
);

// Types that can be zero, but not uninit.
test_panic_msg(
|| mem::uninitialized::<bool>(),
"attempted to leave type `bool` uninitialized, which is invalid"
);
test_panic_msg(
|| mem::uninitialized::<LR>(),
"attempted to leave type `LR` uninitialized, which is invalid"
);
test_panic_msg(
|| mem::uninitialized::<ManuallyDrop<LR>>(),
"attempted to leave type `std::mem::ManuallyDrop<LR>` uninitialized, which is invalid"
);

// Some things that should work.
let _val = mem::zeroed::<bool>();
let _val = mem::zeroed::<LR>();
Expand Down