winapi crashes on x86 because of conflicting stack alignment assumptions

[Trolldemorted]

Debug builds with new rustc versions add an 8 byte alignment check for every 8 byte read/write. MS however says that 8 byte types need to be 4 byte aligned, so those checks will kill your process if the alignment is not 8 (i.e. in approx. 50% of cases).

An example of crashes in downstream projects is GuillaumeGomez/sysinfo#1001.

[RalfJung]

This should be fixed (well, worked around) by rust-lang/rust#112684.

[LunNova]

I'm seeing this with x86_64-pc-windows-gnu cross-compiled on linux

thread 'main' panicked at 'misaligned pointer dereference: address must be a multiple of 0x8 but is 0xc06614', /home/lun/.cargo/registry/src/index.crates.io-6f17d22bba15001f/sysinfo-0.29.4/src/windows/system.rs
:258:34
stack backtrace:
0: rust_begin_unwind
at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library\std\src\panicking.rs:578:5
1: core::panicking::panic_fmt
at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library\core\src\panicking.rs:67:14
2: core::panicking::panic_misaligned_pointer_dereference
at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library\core\src\panicking.rs:174:5
3: <sysinfo::windows::system::System as sysinfo::traits::SystemExt>::refresh_processes_specifics
at /home/lun/.cargo/registry/src/index.crates.io-6f17d22bba15001f/sysinfo-0.29.4/src/windows/system.rs:258:34
4: sysinfo::traits::SystemExt::refresh_processes
at /home/lun/.cargo/registry/src/index.crates.io-6f17d22bba15001f/sysinfo-0.29.4/src/traits.rs:837:9

[LunNova]

Maybe we need #align(4) on this struct. Or maybe downstream code https://github.com/GuillaumeGomez/sysinfo/blob/3991a4de8a8c39aafdedd82b9cfd465cc385060e/src/windows/system.rs#L257 should be using read_unaligned

[RalfJung]

@LunNova which struct are you seeing the issue with?

[ChrisDenton]

Are you running the program in Wine?

[programmerjake]

here's the code: https://docs.rs/sysinfo/0.29.4/x86_64-pc-windows-msvc/src/sysinfo/windows/system.rs.html#258

afaict this is the sysinfo crate's fault since it gets a pointer from Vec<u8> and creates a reference to a not necessarily aligned SYSTEM_PROCESS_INFORMATION inside that Vec<u8> -- this is UB because Vec<u8> only guarantees alignment 1.

[programmerjake]

afaict if you run this code on an old 32-bit arm windows computer it will crash (old enough that it doesn't support misaligned loads), rustc is just exploiting the UB to make it panic more places.

[ChrisDenton]

That is mitigated by the fact the default allocator has a minimum alignment of 16 on x64. So unless a custom allocator is used (that doesn't behave like malloc) then it'll still work in practice.

[programmerjake]

it also manually offsets by an unknown number of bytes (provided by the win32 function), so, because it's crashing, obviously the number of bytes is not a multiple of the alignment -- UB.

[programmerjake]

created a bug: GuillaumeGomez/sysinfo#1009
@LunNova if you could add a code example that uses sysinfo and reproduces the bug that would be awesome!

[ChrisDenton]

it also manually offsets by an unknown number of bytes (provided by the win32 function), so, because it's crashing, obviously the number of bytes is not a multiple of the alignment -- UB.

It should be for any real Windows system. Windows itself will not misalign structures in these buffers (they add padding). Emulators or security software that shims APIs may not properly align structures but that's a bug in them.

[programmerjake]

Winehq aligns to a multiple of 8:
https://gitlab.winehq.org/wine/wine/-/blob/cb33d402bb7fc44a51cd96adfbe740d4b2d7a134/dlls/ntdll/unix/system.c#L2511

[programmerjake]

on x86_64-pc-windows-gnu, ntapi v0.4.1:
std::mem::align_of::<SYSTEM_PROCESS_INFORMATION>() = 8 so winehq should be correct...hmm

[LunNova]

wine + debug build of x86_64-pc-windows-gnu +

    let mut sys = sysinfo::System::new();
    sys.refresh_processes();

is enough to replicate it.

Patching sysinfo with this works around it:

diff --git a/src/windows/system.rs b/src/windows/system.rs
index 849d783..b2b4b17 100644
--- a/src/windows/system.rs
+++ b/src/windows/system.rs
@@ -254,7 +254,8 @@ impl SystemExt for System {
                             .as_ptr()
                             .offset(process_information_offset)
                             as *const SYSTEM_PROCESS_INFORMATION;
-                        let pi = &*p;
+                        let pi = std::ptr::read_unaligned(p);
+                        let pi = π
 
                         process_ids.push(Wrap(p));
 
@@ -278,7 +279,7 @@ impl SystemExt for System {
                     //       able to run it over `process_information` directly!
                     let processes = into_iter(process_ids)
                         .filter_map(|pi| {
-                            let pi = *pi.0;
+                            let pi = std::ptr::read_unaligned(pi.0);
                             let pid = Pid(pi.UniqueProcessId as _);
                             if let Some(proc_) = (*process_list.0.get()).get_mut(&pid) {
                                 if proc_

Wine reports its version as 8.0-staging and was built from

$ nix eval pkgs#lun.wine.src
{ branch = "Proton8-8"; hash = "12mk91smkjlcr7dfjpkdqkql9r98dpxmsl0gmf08avqwppscxljn"; outPath = "/nix/store/2k8hajyi8rm42w9in10msr5p1pjzb0ks-source";
repository = { owner = "GloriousEggroll"; repo = "proton-wine"; type = "GitHub"; }; revision = "d48c657a1364248c1aaed099e5cbee30c1d8c09e"; type = "Git";
url = "https://github.com/GloriousEggroll/proton-wine/archive/d48c657a1364248c1aaed099e5cbee30c1d8c09e.tar.gz"; }

[RalfJung]

That is mitigated by the fact the default allocator has a minimum alignment of 16 on x64. So unless a custom allocator is used (that doesn't behave like malloc) then it'll still work in practice.

Such a custom allocator would be totally legitimate though, so there clearly is a bug here (which I see has been reported at GuillaumeGomez/sysinfo#1009, so I think this is resolved).

[ChrisDenton]

That is mitigated by the fact the default allocator has a minimum alignment of 16 on x64. So unless a custom allocator is used (that doesn't behave like malloc) then it'll still work in practice.

Such a custom allocator would be totally legitimate though, so there clearly is a bug here (which I see has been reported at GuillaumeGomez/sysinfo#1009, so I think this is resolved).

Of course. My point was only that it's unlikely to be the cause of the error in the OP.