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

Fix mem::uninitialized UB warning #100

Merged
merged 1 commit into from
Dec 17, 2019

Conversation

SpaceManiac
Copy link
Contributor

Scary-looking warning fixable as far as I can tell with one-line patch

warning: use of deprecated item 'std::mem::uninitialized': use `mem::MaybeUninit` instead
    --> src/lib.rs:1138:29
     |
1138 |         self.map = unsafe { mem::uninitialized() };
     |                             ^^^^^^^^^^^^^^^^^^
     |
     = note: `#[warn(deprecated)]` on by default

warning: the type `std::collections::HashMap<KeyRef<K>, *mut Node<K, V>, S>` does not permit being left uninitialized
    --> src/lib.rs:1138:29
     |
1138 |         self.map = unsafe { mem::uninitialized() };
     |                             ^^^^^^^^^^^^^^^^^^^^
     |                             |
     |                             this code causes undefined behavior when executed
     |                             help: use `MaybeUninit<T>` instead
     |
     = note: `#[warn(invalid_value)]` on by default
note: std::ptr::NonNull<u8> must be non-null (in this struct field)

Copy link
Contributor

@Gankra Gankra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woah yeah, weird whack code

@Gankra Gankra merged commit 4f681be into contain-rs:master Dec 17, 2019
@SpaceManiac SpaceManiac deleted the fix-ub-warnings branch January 13, 2020 03:25
@RalfJung
Copy link

RalfJung commented May 3, 2020

Any chance of a release with this fix? The crate is still fairly widely used, and with rust-lang/rust#71274 we are turning the UB here into a panic, which breaks the released version of this crate.

@Gankra
Copy link
Contributor

Gankra commented May 6, 2020

Done.

@RalfJung
Copy link

RalfJung commented May 6, 2020

@Gankra looks like something went wrong? I cannot see a new version at https://crates.io/crates/linked-hash-map.

@Gankra
Copy link
Contributor

Gankra commented May 6, 2020

whoops sorry, fixed

@RalfJung
Copy link

RalfJung commented May 6, 2020

Awesome, thanks. :)

@Dylan-DPC-zz
Copy link

@Gankra is there any plan on yanking the older (unsound) versions? This crate has many reverse dependencies yanking the older ones would ensure that they obtain a sound version of this crate instead of bumping the minimum dependency to 0.5.3

@Xanewok
Copy link

Xanewok commented Nov 11, 2020

@Gankra a friendly reminder - it'd be great to yank the previous version as I've been bitten by this as well when using the toml_edit crate and had to scan the backtrace and dig a bit to find that I should bump this transitive dependency to 0.5.3.

Pretty please 🙇 (if not that's fine too ofc, just let us know 🙂)

@RalfJung
Copy link

FWIW, cargo audit should also tell you that this update is necessary.

Xanewok added a commit to paritytech/cargo-unleash that referenced this pull request Nov 13, 2020
quodlibetor added a commit to quodlibetor/materialize that referenced this pull request Nov 23, 2020
There is a long-fixed issue[1] that we hit on this upgrade.

Honestly I'm surprised that dependabot didn't upgrade us on this.

[1]: contain-rs/linked-hash-map#100
Xanewok added a commit to paritytech/cargo-unleash that referenced this pull request Nov 24, 2020
benesch pushed a commit to benesch/materialize that referenced this pull request Nov 24, 2020
There is a long-fixed issue[1] that we hit on this upgrade.

Honestly I'm surprised that dependabot didn't upgrade us on this.

[1]: contain-rs/linked-hash-map#100
austinjones added a commit to austinjones/ttl_cache that referenced this pull request Dec 4, 2020
This commit requires linked-hash-map to include contain-rs/linked-hash-map#100

When linked-hash-map is on v0.5.2, ttl_cache can panic due to the UB checks implemented in Rust 1.48: rust-lang/rust#66151
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

Successfully merging this pull request may close these issues.

5 participants