Removing use of mem::uninitialized from std::io::util #62657
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cramertj (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
src/libstd/sys/cloudabi/mod.rs
Outdated
| @@ -61,7 +59,7 @@ pub use libc::strlen; | |||
|
|
|||
| pub fn hashmap_random_keys() -> (u64, u64) { | |||
| unsafe { | |||
| let mut v = mem::uninitialized(); | |||
| let mut v = mem::MaybeUninit::uninit().assume_init(); | |||
There was a problem hiding this comment.
I feel like a type annotation is definitely warranted here, and probably should've been here before too.
There was a problem hiding this comment.
Yeah, great point. Will update.
There was a problem hiding this comment.
You are just inlining mem::uninitialized here. This does nothing to solve all the problems that mem::uninitialized as.
Please read that assume_init docs. You may only call that method after initialization has completed!
There was a problem hiding this comment.
Makes complete sense. I seem to have misunderstood the ask of the original issue and thought it was simply to replace the deprecated call. I will go back to square one with that in mind and actually fix the problem.
src/libstd/io/util.rs
Outdated
| @@ -44,8 +44,7 @@ pub fn copy<R: ?Sized, W: ?Sized>(reader: &mut R, writer: &mut W) -> io::Result< | |||
| where R: Read, W: Write | |||
| { | |||
| let mut buf = unsafe { | |||
| #[allow(deprecated)] | |||
| let mut buf: [u8; super::DEFAULT_BUF_SIZE] = mem::uninitialized(); | |||
| let mut buf: [u8; super::DEFAULT_BUF_SIZE] = mem::MaybeUninit::uninit().assume_init(); | |||
There was a problem hiding this comment.
I'm afraid this is not the right fix. You are creating uninitialized integers, which under our current rules is UB.
There was a problem hiding this comment.
Thanks for the feedback. I'll try to understand more deeply and fix.
JulianGindi
changed the title
| buf | ||
| }; | ||
|
|
||
| let mut buf: [u8; super::DEFAULT_BUF_SIZE] = [0; super::DEFAULT_BUF_SIZE]; |
There was a problem hiding this comment.
The entire point of this method is to not zero-initialize. See the initializer docs.
You should create a MaybeUninit<[u8; ...]>, and then use &mut *buf.as_ptr_mut() and pass that to initialize. This is, technically, still wrong, but it is less wrong than the previous code. You should add a comment saying that this is UB because we create a reference to uninitialized data, but that we exploit the fact that we are libstd and can rely on more guarantees than "normal" libraries.
Cc rust-lang/unsafe-code-guidelines#90
This is all very subtle. There is a reason I marked the issue as "medium", not "easy". ;)
There was a problem hiding this comment.
You're right, this one is probably outside my skill level currently. I'm sorry for taking up your time, but I do appreciate the bits of knowledge. I'm gonna grab some easier issues to start with. I'm happy to have this PR closed for now.
There was a problem hiding this comment.
That's okay, this happens. :) And no worries, this did not take much time on my end.
Removing use of the deprecated
mem::uninitializedfromCloudABIandstd::io::util. Replaced by usage ofmem::MaybeUninit::uninit().assume_init()Fixes #62397