jamesbirtles / hotkey-rs

A library to listen to global hotkeys in Rust
https://docs.rs/hotkey
55 stars 8 forks source link

`mem::MaybeUninit::uninit().assume_init()` is undefined behavior #4

Open MarkDDR opened 3 years ago

MarkDDR commented 3 years ago

There are a couple places in the code where you do something along the lines of

let mut event: SomeType = mem::MaybeUninit::uninit().assume_init();

This is immediately undefined behavior, regardless of the type. The 2 scenarios I have seen this in the codebase are in the context of a C-style return parameter, so the fix is rather simple.

let mut event: MaybeUninit<SomeType> = mem::MaybeUninit::uninit();
// Maybe attempt to handle a potential int-code error this function may return?
get_event(event.as_mut_ptr());
let event: SomeType = event.assume_init();
koutoftimer commented 3 years ago

@MarkDDR you are absolutely right for general case:

from assume_init

Extracts the value from the MaybeUninit<T> container. This is a great way to ensure that the data will get dropped, because the resulting T is subject to the usual drop handling.

from uninit

Creates a new MaybeUninit<T> in an uninitialized state.

Note that dropping a MaybeUninit<T> will never call T's drop code. It is your responsibility to make sure T gets dropped if it got initialized.

GetMessageW does not expect chuck of memory, it has pointer to, to be in defined state as in case of Rust. It can be allocated with malloc or any other fucntions that just gives you memory with state from previous usage. It is GetMessageW responsibility to write each and every field and ensure that event struct gets initialized right.

winapi::um::winuser::MSG has no pointers in it. If you will drop MaybeUninit<T> and MSG result will be the same.

So what the difference between your solution and author's? - Yours have one more line of code.

Or, in other words, can you supply cases when it is really undefined behavior? According to current code base.

MarkDDR commented 3 years ago

Ok, let me give you a quick primer on undefined behavior. It can do anything, even if it is a primitive type like u8. Take a look at the following code

use std::mem::MaybeUninit;

fn always_returns_true(x: u8) -> bool {
    x < 120 || x == 120 || x > 120
}

fn main() {
    let x: u8 = unsafe { MaybeUninit::<u8>::uninit().assume_init() };
    assert!(always_returns_true(x));
}

playground link

That should always return true right? No matter what value x is, at least one of those conditions should be pass, right? Well unfortunately Undefined Behavior lets the optimizer assume any value it wants for x, including values that contradict previous assumed values of x. Run that code with optimizations turned on and it returns false. Try it yourself!

Now it's not clear if anything that dramatic is necessarily happening in your code base, but why even give it the chance to do something insane like this? Maybe it works now but maybe you change something else unrelated and the optimizer does something sneaky that subtly breaks everything. MaybeUninit currently provides the tools to safely manage uninitialized memory, as I showed above, without the risk of undefined behavior from .uninit().assume_init()