-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
Add example showing UB for uninit Copy types in MaybeUninit::assume_init docs #153030
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -695,6 +695,17 @@ impl<T> MaybeUninit<T> { | |||||
| /// let x_init = unsafe { x.assume_init() }; | ||||||
| /// // `x` had not been initialized yet, so this last line caused undefined behavior. ⚠️ | ||||||
| /// ``` | ||||||
| /// | ||||||
| /// This also applies to simple types that can hold any bit pattern, like integers, boolean and so on. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is unclear what "this" refers to. Also, putting this here makes the entire discussion have a rather strange flow: the "safety" section starts with a basic invariant discussion, then moves on to discuss invariants beyond basic initialization, then has examples for that, and then suddenly moves back to basic invariants. I would suggest just adding a sentence in the paragraph starting "It is up to the caller to guarantee" which emphasizes that this guarantee is non-trivial even for integers.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the feedback @RalfJung , do you think the below paragraph is sound enough?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is not correct, there are types (and parts of types) where uninit memory is fine, such as if
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @RalfJung , I think this information is worth mentioning explicitly if
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no full list the details are not set in stone yet. And I don't think this here is the place for such a list. What is important for the reader to understand is whether and where uninitialized bytes are allowed depends on the type at which the memory is read, and furthermore the integer types do not allow uninitialized bytes (but other types might).
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure will update it thank you for the guidance.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @RalfJung , does this look fine, I used "numeric types" instead of "integers" to include floats (f32, f64) as well, since they also exhibit this behavior. use std::mem::MaybeUninit;
fn main() {
let x = MaybeUninit::<f32>::uninit();
let _ = unsafe { x.assume_init() };
} Compiling playground v0.0.1 (/playground)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.16s
Running `/playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri runner target/miri/x86_64-unknown-linux-gnu/debug/playground`
error: Undefined Behavior: reading memory at alloc179[0x0..0x8], but memory is uninitialized at [0x0..0x8], and this operation requires initialized memory
--> src/main.rs:5:22
|
5 | let _ = unsafe { x.assume_init() };
| ^^^^^^^^^^^^^^^ Undefined Behavior occurred here
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
Uninitialized memory occurred at alloc179[0x0..0x8], in this allocation:
alloc179 (stack variable, size: 8, align: 8) {
__ __ __ __ __ __ __ __ │ ░░░░░░░░
}
note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
error: aborting due to 1 previous error
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think there is a short way to explain what we have to explain, and the long way is already in the type-level docs. So really what we have to achieve here is getting people to read those docs, there's no way around that. I'd suggest something like this as a new paragraph after "It is up to the caller"
Honestly it's probably easier for me to make a PR than to indirectly edit the docs by suggesting changes to you. And anyway I shouldn't approve the PR if I am the one who authored the wording. So I'll make a PR with my proposal. Updating the documentation of tricky unsafe functions is one of the hardest kinds of things to get right so it's not really a great way to get started contributing to the compiler I am afraid. Sorry that you were misled by the "easy" label on that issue.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PR with my proposed changes -- I ended up having to edit the type-level docs as well to really make this work: #153570
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you @RalfJung, will look into other issues cheers, closing this in favor of PR #153570. |
||||||
| /// See the [type-level documentation][inv] for more details. | ||||||
| /// | ||||||
| /// ```rust,no_run | ||||||
| /// use std::mem::MaybeUninit; | ||||||
| /// | ||||||
| /// let x = MaybeUninit::<u8>::uninit(); | ||||||
| /// let x_init = unsafe { x.assume_init() }; // undefined behavior! ⚠️ | ||||||
| /// // Even though `u8` can represent any bit pattern, reading uninitialized memory is still undefined behaviour. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| /// ``` | ||||||
| #[stable(feature = "maybe_uninit", since = "1.36.0")] | ||||||
| #[rustc_const_stable(feature = "const_maybe_uninit_assume_init_by_value", since = "1.59.0")] | ||||||
| #[inline(always)] | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.