microsoft / windows-rs

Rust for Windows
https://kennykerr.ca/rust-getting-started/
Apache License 2.0
10.29k stars 480 forks source link

Bug: undefined behavior in PartialEq implementations #2155

Closed leddoo closed 1 year ago

leddoo commented 1 year ago

Which crate is this about?

windows

Crate version

0.42.0 but doesn't matter, same on master

Summary

some implementations of PartialEq use memcmp. 1) this causes undefined behavior in structs with padding. 2) this puts needless stress on the optimizer (to remove the memcmp, which is worse for small structs).

here's DWRITE_SCRIPT_ANALYSIS, which is where i discovered the issue: it caused non-determinism in my program. replacing the use of eq with field-wise comparison solved the issue (i verified that field-wise comparison & eq disagreed).

pub struct DWRITE_SCRIPT_SHAPES(pub u32);

pub struct DWRITE_SCRIPT_ANALYSIS {
    pub script: u16,
    // undefined_padding: u16, !!!
    pub shapes: DWRITE_SCRIPT_SHAPES,
}

impl ::core::cmp::PartialEq for DWRITE_SCRIPT_ANALYSIS {
    fn eq(&self, other: &Self) -> bool {
        unsafe { ::windows::core::memcmp(self as *const _ as _, other as *const _ as _, core::mem::size_of::<DWRITE_SCRIPT_ANALYSIS>()) == 0 }
    }
}

Toolchain version/configuration

No response

Reproducible example

No response

Crate manifest

No response

Expected behavior

No response

Actual behavior

No response

Additional comments

No response

kennykerr commented 1 year ago

Yes that's a problem - I'll get that fixed up.

kennykerr commented 1 year ago

Here you go: https://github.com/microsoft/windows-rs/pull/2168

Thanks for reporting!