contain-rs / linked-hash-map

A HashMap wrapper that holds key-value pairs in insertion order
https://contain-rs.github.io/linked-hash-map/linked_hash_map/
Apache License 2.0
169 stars 60 forks source link

Fix mem::uninitialized UB warning #100

Closed SpaceManiac closed 4 years ago

SpaceManiac commented 4 years ago

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)
RalfJung commented 4 years ago

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

Gankra commented 4 years ago

Done.

RalfJung commented 4 years ago

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

Gankra commented 4 years ago

whoops sorry, fixed

RalfJung commented 4 years ago

Awesome, thanks. :)

Dylan-DPC-zz commented 4 years ago

@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 commented 3 years ago

@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 :bow: (if not that's fine too ofc, just let us know :slightly_smiling_face:)

RalfJung commented 3 years ago

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