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

Use mem::MaybeUninit instead of mem::uninitialized() since Rust 1.36 #101

Merged
merged 4 commits into from
Aug 22, 2019

Conversation

rhysd
Copy link
Contributor

@rhysd rhysd commented Aug 21, 2019

I noticed that CI fails since this crate is using deprecated std::mem::uninitialized().

https://ci.appveyor.com/project/Stebalien/term/builds/26853323/job/880r4cnu5lhfaccv

This PR uses MaybeUninit which replaces std::mem::uninitialized() using a compiler version switch provided by rustversion crate.

@rhysd rhysd force-pushed the use-maybe-uninit branch from fe47137 to 53ed376 Compare August 21, 2019 12:05
src/win.rs Outdated Show resolved Hide resolved
@rhysd rhysd force-pushed the use-maybe-uninit branch 8 times, most recently from 6bd5ee6 to f9d38fb Compare August 22, 2019 02:06
@rhysd rhysd force-pushed the use-maybe-uninit branch from f9d38fb to 8edc047 Compare August 22, 2019 02:09
@rhysd
Copy link
Contributor Author

rhysd commented Aug 22, 2019

I have addressed the review comment at 8edc047. Instead of uninitialized(), it wraps GetConsoleScreenBufferInfo() as get_console_screen_buffer_info(). After getting uninitialized memory, it immediately initializes the buffer with GetConsoleScreenBufferInfo(). So the returned buffer is guaranteed to be initialized and there is no undefined behavior.

(I apologize many force pushes. I don't have Windows machine so I needed to check Appveyor result instead)

Copy link
Owner

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Thanks but I believe this is still unsafe. See: file:///home/steb/.local/lib/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/share/doc/rust/html/std/mem/union.MaybeUninit.html#examples-1

src/win.rs Outdated
}
#[rustversion::since(1.36)]
unsafe fn get_console_screen_buffer_info(handle: HANDLE) -> io::Result<CONSOLE_SCREEN_BUFFER_INFO> {
let mut buffer_info = ::std::mem::MaybeUninit::uninit().assume_init();
Copy link
Owner

Choose a reason for hiding this comment

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

This needs to call assume_init after filling the buffer. That is, it should call GetConsoleScreenBufferInfo(handle, buffer_info.as_mut_ptr() then buffer_info.assume_init() (if there was no error).

We can't take a reference to uninitialized memory (which is why this type was introduced).

@rhysd
Copy link
Contributor Author

rhysd commented Aug 22, 2019

Ahh, ok, I misunderstood the semantics of MaybeUninit. Thank you for the point.

@rhysd rhysd force-pushed the use-maybe-uninit branch 2 times, most recently from 4fc3a93 to 069fd74 Compare August 22, 2019 04:58
@rhysd rhysd force-pushed the use-maybe-uninit branch from 069fd74 to 65801a0 Compare August 22, 2019 05:02
@rhysd
Copy link
Contributor Author

rhysd commented Aug 22, 2019

I fixed my patch again at 3e9624f and added a new small test for get_console_screen_buffer_info() at 65801a0.

Copy link
Owner

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM.

And yeah, uninitialized values are tricky business...

@Stebalien Stebalien merged commit 97da4f2 into Stebalien:master Aug 22, 2019
@rhysd
Copy link
Contributor Author

rhysd commented Aug 22, 2019

Thank you for correcting me and helping to understand correctly.

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.

2 participants