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

Add option to flag uninitialized integers as UB #1340

Closed
RalfJung opened this issue Apr 16, 2020 · 17 comments · Fixed by #1904
Closed

Add option to flag uninitialized integers as UB #1340

RalfJung opened this issue Apr 16, 2020 · 17 comments · Fixed by #1904
Labels
A-validation Area: This affects enforcing the validity invariant, and related UB checking C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement

Comments

@RalfJung
Copy link
Member

RalfJung commented Apr 16, 2020

The reference currently states that this code is UB, but Miri deliberately accepts it:

fn main() {
    let _val = unsafe { std::mem::MaybeUninit::<usize>::uninit().assume_init() };
}

The reason we accept it is that the lang-team is not sure if we really want this to be UB (also see rust-lang/unsafe-code-guidelines#71), so the conservative choice for the reference is to make it UB for now, but in Miri I fear this might lead to too many errors that people could consider false positives (even though by the letter of the reference they are true positives).

Still it could be interesting to see how much code (that Miri can run) actually fails when considering uninitialized integers UB, so having a flag in Miri to enable stricter checking would be interesting.

@RalfJung RalfJung added C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement A-validation Area: This affects enforcing the validity invariant, and related UB checking labels Apr 16, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Apr 16, 2020

Maybe this is something where we could just emit a warning (silenceable of course) by default?

@RalfJung
Copy link
Member Author

Yeah that also sounds good... but then we have three levels, allow/warn/deny, so it really seems like we should use the lint system.^^

That will also be much harder though, I imagined just having a machine hook that affects whether validation checks integers for being initialized (instead of doing that based on whether we are in "const mode").

@oli-obk
Copy link
Contributor

oli-obk commented Apr 16, 2020

seems good, too. If we make it configurable in miri via the command line, users can turn it off if it bothers them (or we start with opt-in)

@beepster4096
Copy link
Contributor

Looks like this is the place where the check would go

@RalfJung
Copy link
Member Author

Yes that is correct.

@RalfJung
Copy link
Member Author

While at it we should probably also mark ptr-to-int transmutes as UB (Cc rust-lang/unsafe-code-guidelines#286)

@camelid
Copy link
Member

camelid commented Sep 4, 2021

I started working on this, but when I try to use my locally-built rustc with Miri, I get a strange error:

error[E0463]: can't find crate for `rustc_apfloat`
  --> src/lib.rs:10:1
   |
10 | extern crate rustc_apfloat;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't find crate

For more information about this error, try `rustc --explain E0463`.

When I built rustc, it said that it built rustc_apfloat, so I'm not sure what's going on. What should I do to fix this?

@oli-obk
Copy link
Contributor

oli-obk commented Sep 5, 2021

I don't know how to work with a locally built miri except by doing ./x.py test src/tools/miri

It may be that we just don't properly support going the rustup toolchain link route

@RalfJung
Copy link
Member Author

RalfJung commented Sep 5, 2021

@camelid have you followed the instructions at https://github.com/rust-lang/miri/blob/master/CONTRIBUTING.md#building-miri-with-a-locally-built-rustc ? Those are what I used for locally built rustc and they always worked for me (but it's been a few weeks sine I last tried this).

@RalfJung
Copy link
Member Author

RalfJung commented Sep 5, 2021

Hm, actually I am always using stage 2, not stage 1. But you are the one who changed the instructions to recommend stage 1 instead of stage 2... are you saying that does not work any more? Should we revert bb59980?

@camelid
Copy link
Member

camelid commented Sep 5, 2021

Hm, actually I am always using stage 2, not stage 1. But you are the one who changed the instructions to recommend stage 1 instead of stage 2... are you saying that does not work any more? Should we revert bb59980?

Yes, I'm almost done with a stage2 build to see if that fixes it. If that works, I was planning to also suggest we revert that commit :)

@camelid
Copy link
Member

camelid commented Sep 5, 2021

Indeed, that fixed it.

@camelid
Copy link
Member

camelid commented Sep 5, 2021

are you saying that does not work any more?

I'm not sure; I wonder if I had already built a full stage2 when I was testing it for that PR and so it didn't work then either. Bootstrapping stages are pretty confusing ;)

bors added a commit that referenced this issue Sep 6, 2021
Stage 2 seems to be required after all

Reverts most of bb59980.

See the discussion starting at #1340 (comment) for more.
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 10, 2021
miri: Detect uninitialized integers and floats

Part of rust-lang/miri#1340.

Companion Miri PR: rust-lang/miri#1904

r? `@RalfJung`
@bors bors closed this as completed in a8b976e Nov 10, 2021
camelid added a commit to camelid/rust that referenced this issue Nov 10, 2021
This is the last step in landing rust-lang/miri#1340!
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 10, 2021
Update Miri

Fixes rust-lang#90763.

This is the last step in landing rust-lang/miri#1340!

r? `@RalfJung`
@RalfJung
Copy link
Member Author

The good news is that the Rust libcore/liballoc test suites pass even with this flag. :)

@beepster4096
Copy link
Contributor

That doesn't sound correct to me, I know at least std::io::copy and std::io::Read::read_to_end create slices of uninit u8s.

@bjorn3
Copy link
Member

bjorn3 commented Nov 14, 2021

This option doesn't check behind references AFAIK, so it doesn't catch slices of uninit u8s.

@RalfJung
Copy link
Member Author

Yeah -- (a) I am not sure if we are hitting those codepaths (we are not running the std test suite, only core and alloc), and (b) validity is not checked recursively through references (see rust-lang/unsafe-code-guidelines#77).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-validation Area: This affects enforcing the validity invariant, and related UB checking C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants