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

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

Closed
programmerjake opened this issue Jan 23, 2020 · 9 comments
Assignees

Comments

@programmerjake
Copy link

programmerjake commented Jan 23, 2020

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
Copy link
Collaborator

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

@novacrazy novacrazy self-assigned this Jan 23, 2020
@novacrazy
Copy link
Collaborator

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
Copy link
Contributor

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

@novacrazy
Copy link
Collaborator

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
Copy link
Contributor

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

@novacrazy
Copy link
Collaborator

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

@Robbepop
Copy link
Contributor

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

@novacrazy
Copy link
Collaborator

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

@RalfJung
Copy link

RalfJung commented May 3, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants