Add example showing UB for uninit Copy types in MaybeUninit::assume_init docs#153030
Add example showing UB for uninit Copy types in MaybeUninit::assume_init docs#153030bp7968h wants to merge 1 commit intorust-lang:mainfrom
Conversation
rustbot
added
S-waiting-on-review
|
r? @joboet rustbot has assigned @joboet. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
bp7968h
force-pushed
the
docs-issue-150689
branch
from
234ebdc to
82e33d1
Compare
|
r? @RalfJung |
|
|
|
I don't think the function level docs should be super detailed; if anything that'd be a task for the type-level docs. |
| /// | ||
| /// 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. |
There was a problem hiding this comment.
| /// // Even though `u8` can represent any bit pattern, reading uninitialized memory is still undefined behaviour. | |
| /// // Even though `u8` can represent any bit pattern, reading uninitialized memory is still undefined behavior. |
| /// // `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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thank you for the feedback @RalfJung , do you think the below paragraph is sound enough?
It is up to the caller to guarantee that the `MaybeUninit<T>` really is in an initialized
state. Calling this when the content is not yet fully initialized causes immediate undefined
behavior. This is true even for types like integers that can hold any bit pattern, meaning
reading uninitialized memory is always undefined behavior. The [type-level documentation][inv]
contains more information about this initialization invariant.
There was a problem hiding this comment.
meaning
reading uninitialized memory is always undefined behavior
This is not correct, there are types (and parts of types) where uninit memory is fine, such as if T (the inner type) is itself MaybeUninit, or if the byte is a padding byte.
There was a problem hiding this comment.
Hi @RalfJung , I think this information is worth mentioning explicitly if byte is a padding byte and T (the inner type) is itself MaybeUninit are the only ones where uninit memory. However, if there are more it's still worth stating them all or point a docs what those types are.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Sure will update it thank you for the guidance.
There was a problem hiding this comment.
It is up to the caller to guarantee that the `MaybeUninit<T>` really is in an initialized
state. Calling this when the content is not yet fully initialized causes immediate undefined
behavior. This includes numeric types, even though they can hold any bit pattern
once initialized, reading uninitialized memory at a numeric type is undefined behavior.
The [type-level documentation][inv] contains more information about this initialization invariant.
@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 errorThere was a problem hiding this comment.
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"
It is a common mistake to assume that this function is safe to call on integers because they can hold all bit patterns. It is also a common mistake to think that calling this function is UB if any byte is uninitialized. Both of these assumptions are wrong. If that is surprising to you, please read the type-level documentation.
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.
There was a problem hiding this comment.
PR with my proposed changes -- I ended up having to edit the type-level docs as well to really make this work: #153570
There was a problem hiding this comment.
Thank you @RalfJung, will look into other issues cheers, closing this in favor of PR #153570.
rustbot
removed
the
S-waiting-on-review
5 participants
This PR adds an explicit example and explanation showing that calling
assume_init()on uninitialized memory is undefined behavior even for simple types like integers that can hold any bit pattern.Following up on the discussion in #150689 - waited for @Sekar-C-Mca to respond as suggested by @RalfJung.
Closes #150689