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

Initializing struct fields with MaybeUninit #62965

Closed
Osspial opened this issue Jul 24, 2019 · 15 comments
Closed

Initializing struct fields with MaybeUninit #62965

Osspial opened this issue Jul 24, 2019 · 15 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. F-raw_ref_op `#![feature(raw_ref_op)]` T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@Osspial
Copy link

Osspial commented Jul 24, 2019

The MaybeUninit docs have one weird, concerning section:

There is currently no supported way to create a raw pointer or reference to a field of a struct inside MaybeUninit<Struct>. That means it is not possible to create a struct by calling MaybeUninit::uninit::<Struct>() and then writing to its fields.

However, there seems to be a pretty clear, simple way to do that:

use std::mem::MaybeUninit;

fn main() {
    struct Foo {a: u32, b: bool}
    let mut idk: MaybeUninit<Foo> = MaybeUninit::uninit();
    unsafe{ (*idk.as_mut_ptr()).b = true; }
}

If you run that through MIRI, it doesn't complain, which strongly implies that the documentation is incorrect: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=1862e046da3dbd485f83fd8ffa1ce4fd

So, is the documentation incorrect, or is MIRI incorrectly accepting the above code as defined behavior?

(It's worth noting that MIRI correctly rejects using mem::uninitialized() in that context: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=02e33fd2582dcaeaf2b97e9e0db7e0d1)

cc #53491 @RalfJung

@solb

This comment has been minimized.

@Centril
Copy link
Contributor

Centril commented Jul 25, 2019

If you run that through MIRI, it doesn't complain, which strongly implies that the documentation is incorrect: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=1862e046da3dbd485f83fd8ffa1ce4fd

The assumption here that if Miri doesn't complain then there is no UB is not correct. Rust does not have a specification, but until the language team decides otherwise, the snippet above is UB even though Miri does not complain and even though LLVM might not exploit it as UB. And even if we decide otherwise, Miri might still not complain as it is not necessarily a sound analysis.

The documentation above is very much intentional and correct. If it stops being correct it will be removed.

@thomcc
Copy link
Member

thomcc commented Jul 25, 2019

This can be UB because writing the fields like that will trigger a drop of the uninitialized value that was present before: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=756bfd2040f8f50d9d8d0081ba1027f6

@hellow554
Copy link
Contributor

@solb please use the "subscribe" button to the right, instead of writing a comment.

@RalfJung
Copy link
Member

The assumption here that if Miri doesn't complain then there is no UB is not correct.

Indeed. To quote from the Miri README:

There are still plenty of open questions around the basic invariants for some types and when these invariants even have to hold. Miri tries to avoid false positives here, so if you program runs fine in Miri right now that is by no means a guarantee that it is UB-free when these questions get answered. In particular, Miri does currently not check that integers are initialized or that references point to valid data.

That said, the code in the OP actually is not UB according to my understanding. However, as @thomcc pointed out, it only works for struct fields that do not have drop glue.

Still, non-drop-glue-types are a common case for FFI. So this might be worth mentioning in the docs?

@solb
Copy link

solb commented Jul 26, 2019

@thomcc Agreed, but wouldn't writing the struct member using std::ptr::write() or the pointer type's write() method be safe?

@RalfJung
Copy link
Member

@solb no, unfortunately not. To create a raw pointer you have to write &mut foo.field, and at that moment you caused UB because producing a reference to something that is not a valid value of the given type is UB. Whether this should be or not is being discussed here, but until we made a decision (and RFC'd that) we have to treat it as UB.

@jonas-schievink jonas-schievink added A-miri Area: The miri tool C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Aug 6, 2019
@RalfJung RalfJung removed the A-miri Area: The miri tool label Aug 7, 2019
@RalfJung
Copy link
Member

RalfJung commented Aug 7, 2019

@jonas-schievink I removed the Miri label; the bug is about it being (in general) impossible in Rust to write to a field of an uninitialized struct. There is no Miri bug here.
I don't know which A-* label fits, though. A-unsafe or A-language-spec or A-abstract-machine don't exist. ;)

@jonas-schievink
Copy link
Contributor

@RalfJung Sure thing! We lack a lot of A- labels to express things like this, so I often have to guess the closest matching one.

@RalfJung RalfJung added the F-raw_ref_op `#![feature(raw_ref_op)]` label Feb 29, 2020
@RalfJung
Copy link
Member

A raw reference operator is now available on nightly -- once stabilizied, that would solve this issue.

Also see the tracking issue: #64490

@piegamesde
Copy link
Contributor

Until this is resolved: would transmuting from a version of the structs where all fields are MaybeUninit be defined behavior?

use std::mem::MaybeUninit;

fn main() {
    struct Foo {a: u32, b: bool}
    struct FooInitHelper { a: MaybeUninit<u32>, b: MaybeUninit<bool> };
    let mut idk = FooInitHelper { a: MaybeUninit::uninit(), b: MaybeUninit::uninit() };
    // Initialize the values here
    let mut idk: Foo = std::mem::transmute(idk);
}

@thomcc
Copy link
Member

thomcc commented Apr 15, 2020

Only if the type is #[repr(C)], and even then I'm not sure.

@RalfJung
Copy link
Member

With repr(C) those two structs have the same field order and offsets, so yes, that would be an option. But with the implicit repr(Rust), the fields might be ordered differently.

@memoryruins
Copy link
Contributor

With the stabilization of std::ptr::addr_of_mut in 1.51, initializing a struct field-by-field with MaybeUninit is now possible https://doc.rust-lang.org/std/mem/union.MaybeUninit.html#initializing-a-struct-field-by-field

@Mark-Simulacrum
Copy link
Member

Given the documentation cited above (https://doc.rust-lang.org/std/mem/union.MaybeUninit.html#initializing-a-struct-field-by-field), I think this can be closed. There could still be desire for a safe way to write to fields behind MaybeUninit, but that seems like out of scope for an issue to me (would need RFC, etc.).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. F-raw_ref_op `#![feature(raw_ref_op)]` T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants