Rust: Uplift `MaybeUninit::uninit().assume_init()` lint from clippy

Created on 27 Aug 2020  路  8Comments  路  Source: rust-lang/rust

Using MaybeUninit::uninit().assume_init() is instant UB unless the target type is itself composed entirely of MaybeUninit (eg: [MaybeUninit<u8>; 10], or similar).

It's also the most common thing for a person to try when using MaybeUninit for the first time. I don't think that Rust has any footgun bigger than this single expression.

Clippy has a lint against this, and the lint needs to be uplifted into the compiler.

Essentially the lint is: if the target type of this expression isn't itself some sort of MaybeUninit, then deny by default.

A-lint C-feature-request T-lang

Most helpful comment

We already have a lint for this: invalid_values.

Using MaybeUninit::uninit().assume_init() is instant UB unless the target type is itself composed entirely of MaybeUninit (eg: [MaybeUninit; 10], or similar).

That's not true. Any union instead of MaybeUninit will also work. And ZST are not insta-UB either.

All 8 comments

Even the [MaybeUninit<u8>; 10] example wouldn't need to use this pattern if uninit_array were to stabilize.

Ideally once there's a complete confidence that there's no false positives, we upgrade the deny by default into either a hard error, or maybe into an instant runtime panic.

But that might be a controversial step?

We already have a lint for this: invalid_values.

Using MaybeUninit::uninit().assume_init() is instant UB unless the target type is itself composed entirely of MaybeUninit (eg: [MaybeUninit; 10], or similar).

That's not true. Any union instead of MaybeUninit will also work. And ZST are not insta-UB either.

I am working on strengthening that existing lint in https://github.com/rust-lang/rust/pull/71274. But there's a lot of crater regressions, so help would be appreciated.

Fair point on unions and zsts

I am working on strengthening that existing lint in #71274. But there's a _lot_ of crater regressions, so help would be appreciated.

Ouch, that was silly of me... we actually have two things that try to guard against such bugs, a lint and runtime panics. However, the runtime panics only apply to mem::uninitialized(), not to MaybeUninit::uninit().assume_init(), as the latter case cannot easily be detected at runtime. The PR is about the latter.


So regarding the lint, I think it is in a pretty good shape already. The one thing it does not do is warn about uninitialized integers, raw pointers and floats. The reason for this is that https://github.com/rust-lang/unsafe-code-guidelines/issues/71 is still open, so I didn't feel like we should already assert that uninit ints are UB when the lang team has not firmly made that decision yet.

If there are any other cases of MaybeUninit::uninit().assume_init() that rustc does not warn about, please let me know. Some enums are tricky to handle, but with concrete examples we can make the lint properly understand more of them.

@Lokathor what is the actionable part of this issue?

As I said above, we already have such a lint in rustc, and I'd be happy to improve it given examples where it should fire but does not.
But I checked the clippy lint, and I think it is way too aggressive for a rustc lint -- it warns about many cases that are definitely fine (like an uninitialized repr(transparent) newtype around MaybeUninit), and in cases where it cannot know if things are actually problematic (like when there are generics involved).

It also warns about some cases that are okay with the current rustc layout strategy but not if that ever changes, like

enum Univariant {
  V(MaybeUninit<bool>)
}

I wanted to be conservative when adding the lint and not turn these into errors, but if T-lang is on board I would be fine with triggering the lint for all enums.

Well, alright then.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

withoutboats picture withoutboats  路  202Comments

Leo1003 picture Leo1003  路  898Comments

nikomatsakis picture nikomatsakis  路  259Comments

nikomatsakis picture nikomatsakis  路  331Comments

GuillaumeGomez picture GuillaumeGomez  路  300Comments