Replace `std::mem::uninitialized` with `MaybeUninit` by murarth · Pull Request #1027 · rust-windowing/winit

murarth · 2019-07-08T02:44:13Z

Closes #1025

Tested on all platforms changed
cargo fmt has been run on this branch
Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
Created or updated an example program if it would help users understand this functionality
Updated feature matrix, if new features were added or implemented

murarth · 2019-07-08T02:47:55Z

All uses of mem::uninitialized were in Xlib FFI code, so Linux X11 is the only platform affected by this change.

src/platform_impl/linux/mod.rs

src/platform_impl/linux/x11/mod.rs

src/platform_impl/linux/x11/util/client_msg.rs

src/platform_impl/linux/x11/util/input.rs

src/platform_impl/linux/x11/util/window_property.rs

src/platform_impl/linux/x11/window.rs

goddessfreya · 2019-07-08T05:09:06Z

~~I'm not sure who thought MaybeUninit is better than mem::uninitialized... it seams easy to mess up, unlike the prior mem::uninitialized api...~~

murarth · 2019-07-08T06:01:53Z

I was not aware that even an uninitialized i32 is undefined behavior. I'll address those issues accordingly.

felixrabe · 2019-07-08T07:51:15Z

~~I'm not sure who thought MaybeUninit is better than mem::uninitialized... it seams easy to mess up, unlike the prior mem::uninitialized api...~~

You know you can "delete" a comment :)

murarth · 2019-07-08T16:58:09Z

src/platform_impl/linux/x11/util/input.rs

@@ -25,16 +25,16 @@ impl From<ffi::XIModifierState> for ModifiersState {

 pub struct PointerState<'a> {


It turns out that some of these private fields are unused. rustc wasn't detecting this before because the struct was being filled with &mut pointer_state.field references and this was seen as a use. I've commented out the fields as it should have no effect on the program. I could instead make them public or add #[allow(dead_code)] to the struct.

I kept them in both 1) to make allocation easier, and 2) so that contributors can more easily discover/utilize functionality they might need (the idea is to provide a robust internal API). I'd prefer they be kept in, though with comments explaining the rationale.

Fair enough. I've restored the fields and marked them pub to silence the warnings.

murarth · 2019-07-08T19:50:50Z

@zegentzy I believe I have resolved all of the undefined behavior issues.

aloucks · 2019-07-08T21:03:30Z

There should probably be a more thorough audit of mem::uninitialized.

I think that most of these cases should either be replaced with mem::zeroed() (e.g. for native struct initialization) or simply use 0 or ptr::null_mut() to initialize integers and pointers. Arrays should probably just use [0; the_size] as well. Uninitialized memory is playing with fire and a recipe for UB. We should try to avoid MaybeUninit unless it's absolutely necessary.

august64 · 2019-07-08T23:17:06Z

@aloucks in what ways is zeroed safer than uninitialized?

goddessfreya

This code looks about right. Can we try replacing some of them with MaybeUninit::zeroed()?

goddessfreya · 2019-07-08T23:29:23Z

@francesca64 Reading from unitialized memory triggers UB. Reading from zero-initialized memory only triggers UB if that is not a valid state. Since most (all?) of these variables are integers, accidentally reading from them if they get zeroed-initialized, instead of uninitialized, will help us avoid accidental UB.

aloucks · 2019-07-08T23:34:41Z

@aloucks in what ways is mem::zeroed safer than MaybeUninit?

In general, you should avoid mem::zeroed() too, but sometimes it's you're only option. (e.g. https://github.com/retep998/winapi-rs#how-do-i-create-an-instance-of-a-union). It's usually fine for initializing native structs (because they generally don't contain references).

Why would you prefer MaybeUninit over initializing pointers and integers to zero/null?

aloucks · 2019-07-08T23:59:02Z

Reading from zero-initialized memory only triggers UB if that is not a valid state.

I don't think you even have to read from it for it to be considered UB. The addition of MaybeUninit doesn't make uninitialized memory safe. It was just impossible to use mem::uninitialized() in some cases before without triggering UB (e.g. with bools). MaybeUninit provides a for a way to to do it safely, but it's still extremely easy to get it wrong.

goddessfreya · 2019-07-09T00:33:13Z

@aloucks According to the docs:

It depends on T whether that already makes for proper initialization. For example, MaybeUninit::zeroed() is initialized, but MaybeUninit<&'static i32>::zeroed() is not because references must not be null.

So, operations that would normally trigger UB on uninitialized memory will not trigger UB on zero-initialized types, if zero is a valid initialization of the type.

src/platform_impl/linux/x11/mod.rs

src/platform_impl/linux/x11/util/geometry.rs

src/platform_impl/linux/x11/util/input.rs

goddessfreya · 2019-07-09T00:37:58Z

Why would you prefer MaybeUninit over initializing pointers and integers to zero/null?

I'd rather we zero initialize whole integer-only structs than manually set their contents to zero.

aloucks · 2019-07-09T01:07:28Z

So, operations that would normally trigger UB on uninitialized memory will not trigger UB on zero-initialized types, if zero is a valid initialization of the type.

Yes, absolutely - that's why I was saying prefer it over uninitialized and prefer 0 or ptr::null_mut() for non-struct members.

src/platform_impl/linux/x11/mod.rs

aloucks · 2019-07-09T01:26:00Z

MaybeUninit::zeroed().assume_init() is no different than mem::zeroed().

murarth · 2019-07-09T01:34:27Z

MaybeUninit::zeroed().assume_init() is no different than mem::zeroed().

Yes, but since mem::uninitialized has been deprecated, mem::zeroed may also be deprecated in the future and using MaybeUninit::zeroed now avoids making another change in the future.

That said, if brevity is preferred, I'm not opposed to changing it.

aloucks · 2019-07-09T02:27:16Z

What I had originally meant by auditing the use of mem::uninitialized() is that many of these cases don't require it at all and can actually be expressed with safe code.

For example, XConnection::query_pointer can be implemented without it and the only unsafe part is where it calls the xinput2.XIQueryPointer ffi function.

pub fn query_pointer(
    &self,
    window: ffi::Window,
    device_id: c_int,
) -> Result<PointerState<'_>, XError> {
    let mut root = 0;
    let mut child = 0;
    let mut root_x = 0.0;
    let mut root_y = 0.0;
    let mut win_x = 0.0;
    let mut win_y = 0.0;
    let mut group = Default::default();
    let mut buttons = Default::default();
    let mut modifiers = Default::default();

    let relative_to_window = unsafe { (self.xinput2.XIQueryPointer)(
        self.display,
        device_id,
        window,
        &mut root,
        &mut child,
        &mut root_x,
        &mut root_y,
        &mut win_x,
        &mut win_y,
        &mut buttons,
        &mut modifiers,
        &mut group,
    ) == ffi::True };

    if let Err(err) = self.check_errors() {
        Err(err)
    } else {
        Ok(PointerState {
            xconn: self,
            root,
            child,
            root_x,
            root_y,
            win_x,
            relative_to_window,
            buttons,
            modifiers,
            win_y,
            group
        })
    }
}

Interestingly enough now that we're creating that struct with 100% safe code we find that most of those fields aren't even being used. The overzealous usage of unsafe hid them from the compiler. We shouldn't blindly swap mem::uninitialized with MaybeUninit, instead we should remove it's usage everywhere that isn't 100% necessary.

warning: field is never used: `root`
  --> src/platform_impl/linux/x11/util/input.rs:28:5
   |
28 |     root: ffi::Window,
   |     ^^^^^^^^^^^^^^^^^
   |
   = note: #[warn(dead_code)] on by default

warning: field is never used: `child`
  --> src/platform_impl/linux/x11/util/input.rs:29:5
   |
29 |     child: ffi::Window,
   |     ^^^^^^^^^^^^^^^^^^

warning: field is never used: `win_x`
  --> src/platform_impl/linux/x11/util/input.rs:32:5
   |
32 |     win_x: c_double,
   |     ^^^^^^^^^^^^^^^

warning: field is never used: `win_y`
  --> src/platform_impl/linux/x11/util/input.rs:33:5
   |
33 |     win_y: c_double,
   |     ^^^^^^^^^^^^^^^

warning: field is never used: `group`
  --> src/platform_impl/linux/x11/util/input.rs:36:5
   |
36 |     group: ffi::XIGroupState,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^

warning: field is never used: `relative_to_window`
  --> src/platform_impl/linux/x11/util/input.rs:37:5
   |
37 |     relative_to_window: bool,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^

* Prefer safe zero-initialization using `Default`, when possible * Zero-initialize integers and floats using `0` or `0.0` * Use `MaybeUninit::uninit` for large byte buffers and union types * Use `MaybeUninit::uninit` when the resulting value is ignored

murarth · 2019-07-09T17:19:32Z

I've pushed a commit reforming usage of MaybeUninit, following these guidelines:

Prefer safe zero-initialization using Default, when possible
Zero-initialize integers and floats using 0 or 0.0
Use MaybeUninit::uninit for large byte buffers and union types
Use MaybeUninit::uninit when the resulting value is ignored

aloucks · 2019-07-10T01:07:44Z

src/platform_impl/linux/mod.rs

    if let Ok(ref xconn) = *xconn_lock {
-        let mut buf: [c_char; 1024] = mem::uninitialized();
+        // `assume_init` is safe here because the array consists of `MaybeUninit` values,
+        // which do not require initialization.


This could be replaced with

let mut buf = [0 as c_char; 1024];

Why zero-initialize a KB of memory? If we can't trust a C library to copy data into a buffer and accurately report the number of bytes written, why are we using C libraries at all?

aloucks · 2019-07-10T01:13:12Z

src/platform_impl/linux/x11/util/hint.rs

    pub fn get_normal_hints(&self, window: ffi::Window) -> Result<NormalHints<'_>, XError> {
        let size_hints = self.alloc_size_hints();
-        let mut supplied_by_user: c_long = unsafe { mem::uninitialized() };
+        let mut supplied_by_user = MaybeUninit::uninit();


This is just an integer. There is no need for uninit.

You can just set it normally (and change the param back to &mut supplied_by_user below:

let mut supplied_by_user = 0;

The value is ignored. There's no unsafe. There's no undefined behavior.

Why zero-initialize a variable that is never read?

aloucks · 2019-07-10T01:14:17Z

src/platform_impl/linux/x11/util/input.rs

-            let (_, _, new_count) = self.lookup_utf8_inner(ic, key_event, &mut buffer);
+        // `assume_init` is safe here because the array consists of `MaybeUninit` values,
+        // which do not require initialization.
+        let mut buffer: [MaybeUninit<u8>; TEXT_BUFFER_SIZE] =


This could be replaced with

let mut buffer = [0_u8; TEXT_BUFFER_SIZE];

aloucks · 2019-07-10T01:15:04Z

src/platform_impl/linux/x11/window.rs

-                // 64 would suffice for Linux, but 256 will be enough everywhere (as per SUSv2). For instance, this is
-                // the limit defined by OpenBSD.
-                const MAXHOSTNAMELEN: usize = 256;
-                let mut hostname: [c_char; MAXHOSTNAMELEN] = mem::uninitialized();


This could be replaced with

let mut hostname = [0 as c_char; MAXHOSTNAMELEN];

aloucks · 2019-07-10T01:16:29Z

src/platform_impl/linux/x11/window.rs

        let cursor = unsafe {
            // We don't care about this color, since it only fills bytes
            // in the pixmap which are not 0 in the mask.
-            let dummy_color: ffi::XColor = mem::uninitialized();


Why not mem::zeroed() and be done with it? (or Default::default(), which is probably the same)

Same as above, the value is not used.

src/platform_impl/linux/x11/window.rs

goddessfreya

Assuming https://github.com/rust-windowing/winit/pull/1027/files/15409a1600103be3899aba7d6a576a48bedfe5c0#diff-957c45ae2e25aaf20b1e6229cc7aa195R423 can't be changed to use Default, this lgtm.

murarth · 2019-07-11T15:52:11Z

Apologies if I've missed something. Are there any unresolved issues with this PR?

…1027) * Replace `std::mem::uninitialized` with `MaybeUninit` * Avoid undefined behavior when using `MaybeUninit` * Restore unused `PointerState` fields as internally public * Zero-initialize some struct values in Xlib FFI calls * Reform usage of `MaybeUninit` in Xlib FFI * Prefer safe zero-initialization using `Default`, when possible * Zero-initialize integers and floats using `0` or `0.0` * Use `MaybeUninit::uninit` for large byte buffers and union types * Use `MaybeUninit::uninit` when the resulting value is ignored

…ndowing#1027)" This reverts commit 7daf146.

Replace std::mem::uninitialized with MaybeUninit

bd60b8c

goddessfreya suggested changes Jul 8, 2019

View reviewed changes

Avoid undefined behavior when using MaybeUninit

1560f66

murarth commented Jul 8, 2019

View reviewed changes

Restore unused PointerState fields as internally public

4babbbc

goddessfreya reviewed Jul 8, 2019

View reviewed changes