fizyk20/generic-array

mem::uninitialized is nearly always UB, switch to MaybeUninitialized or something else

Closed this issue · 9 comments

programmerjake commented

mem::uninitialized is deprecated because it's basically impossible to use correctly:

From mem::uninitialized's docs:

The reason for deprecation is that the function basically cannot be used
correctly: the Rust compiler assumes that values are properly initialized.
As a consequence, calling e.g. mem::uninitialized::<bool>() causes immediate
undefined behavior for returning a bool that is not definitely either true
or false. Worse, truly uninitialized memory like what gets returned here
is special in that the compiler knows that it does not have a fixed value.
This makes it undefined behavior to have uninitialized data in a variable even
if that variable has an integer type.
(Notice that the rules around uninitialized integers are not finalized yet, but
until they are, it is advisable to avoid them.)

novacrazy commented

I've been meaning to do this, and will try to get to it soon. Thanks for reminding me.

  • 👍1
novacrazy commented

Ugh, I hadn't touched the repo in a few months and wasn't paying attention to which remote I was pushing to, so now the initial implementation with MaybeUninit is at 289ee56. Oh well.

@fizyk20 Do you have any feedback for this implementation?

A few things of note:

  1. I messed with the private transmute implementation to use ManuallyDrop rather than mem::forget. With any opt level >0 it compiles to the same machine code, and is slightly more efficient with no optimizations.
  2. Similar to 1, I rewrote the sequence traits using MaybeUnint and ManuallyDrop instead of mem::forget
  3. Replaced N::to_usize() with N::USIZE

Technically these are not breaking changes, and do not affect the external API.

Robbepop commented

From what I can tell this has already been fixed and merged to master so we could technically close this, right?

novacrazy commented

I was waiting on @fizyk20 's response, but they don't seem active on this repo anymore. I'll see about double-checking the code and pushing a version update when I have time in the next few days.

Robbepop commented

When @fizyk20 is no longer active on the repo, how can we effectively update the crate on crates.io?

novacrazy commented

I have the permissions to do that, so no worries.

  • 👍1
Robbepop commented

That's good to hear because it would be really bad for this amazing crate!

novacrazy commented

This should be fixed in the 0.14.0 release. Please reopen if issues persist.

RalfJung commented

Is there any chance of a 0.13.x (semver-compatible) release with the fix? This bug breaks e.g. https://crates.io/crates/async-local-bounded-channel, and with rust-lang/rust#71274 the silent UB will turn into a loud panic.

  • 👍1