std::fs::remove_dir_all occasionally panics on Windows with assertion failed: info.is_aligned()

[badboy]

remove_dir_all panics on some Windows version when calling std::fs::remove_dir_all due to a newly introduced assert!.

This code:

std::fs::remove_dir_all(&pings_dir)?;

I expected to see this happen: It removes everything or reports an error.

Instead, this happened: It panics with assertion failed: info.is_aligned().

Meta

$ rustc --version
rustc 1.65.0 (897e37553 2022-11-02)

(Crashes on Windows, but we're just using upstream 1.65.0 there).

Full Firefox crash report: https://crash-stats.mozilla.org/report/index/02651884-5b3a-488e-9a43-994b80221106
Firefox bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1799442

Last commit that changed that: c41f21b
PR: #101171
File: https://github.com/rust-lang/rust/blob/master/library/std/src/sys/windows/fs.rs#L737-L739

We saw that on 3 Windows 10 versions: 10.0.18362, 10.0.18363 and 10.0.19044.
Though I haven't checked the latest crashes, it might be more now.

cc @thomcc

[thomcc]

Oof, this sucks! I don't know why this wouldn't be unaligned. As the comment there states, the function's docs state that it should be aligned.

While we could use an unaligned read for the various fields, this would only reduce the alignment requirement to 2bytes. There's not much we can do for the filename array, since we need to provide a &[u16] to the caller. Hopefully that's enough... I'll do this after work.

CC @ChrisDenton who may have some ideas.

[ChrisDenton]

This is concerning to me, for more than just Rust. These structs should really be aligned as documented.

@badboy Do you happen to have any more context for the crashes? My best guess is this may ultimately be caused by a bad filesystem driver (e.g. certain VM shared drives and RAM drives are particularly notorious). If it's the OS itself then that's a bigger issue. Also I'm assuming the crashes are on actual Windows and not WINE or other emulation?

In any case, yeah we'll need to weaken our assumption here which is unfortunate.

[thomcc]

I went through the other places I asserted alignment in #101171, and they should be fine -- those are entirely under our control (unlike this where I relied on the system behaving as documented), so I don't think there's any need to remove them too.

[badboy]

This is concerning to me, for more than just Rust. These structs should really be aligned as documented.

@badboy Do you happen to have any more context for the crashes? My best guess is this may ultimately be caused by a bad filesystem driver (e.g. certain VM shared drives and RAM drives are particularly notorious). If it's the OS itself then that's a bigger issue. Also I'm assuming the crashes are on actual Windows and not WINE or other emulation?

In any case, yeah we'll need to weaken our assumption here which is unfortunate.

From all I see in the Firefox crash reports these are actual machines.
It's definitely buggy behavior compared to the docs, but I don't know where that bug is (Windows bug, driver bug, broken memory/disk, ...)

[badboy]

According to our bots this is a topcrasher on Firefox Nightly.
We might have a workaround there (avoiding that specific codepath overall), it should land with the next nightly, so I will know more after the weekend.

[mati865]

Looking at the report in OP there are at least 2 DLLs that could have caused the issue: SuRun (SuRunExt.dll) that restricts programs permissions and Sandboxie (SbieDll.dll) which is a some kind of a sandbox. I have never used either of them but given their functionality it sounds they could be the reason of this misalignment (I think it's even UB to misalign pointer that is documented as aligned...).

[thomcc]

It being a top crasher is worrying (even if Firefox can work around it, Firefox is not the only Rust application on Windows). That said, if this only impacts sandboxed processes it'd be less of a concern.

[ChrisDenton]

Would anyone be willing to report this to sandboxie?

[thomcc]

I took a look and it looks like their issue tracker has template that seems pretty annoying to fill out. I think someone at Mozilla might have sufficient info to file the report, so I'm inclined to punt it to them.

I haven't looked very closely but much of the code in
https://github.com/sandboxie-plus/Sandboxie/blob/c8484083f10bd5d126cb6c8b7a68bcdf9ecf187a/Sandboxie/core/dll/file_dir.c doesn't seem to be handling alignment, which might be the issue. That said it might be something else, this is just from a really quick skim.

This probably makes this issue a lot less high priority, if it's just a bug in the sandboxing library Firefox is using.

[glandium]

Sandboxie is not used by Firefox itself. That would be something random people install on their own.

[glandium]

(but indeed, there's a 100% correlation on the presence of SbieDll.dll (if you look at the correlations tab on the crash report))

[thomcc]

Either way I think we should keep the alignment-insensitive code I've added in #104558, since I do believe it could happen for other similar kinds of things to sandboxie, or even buggy drivers.

I think it's probably relatively low priority though, unless I'm misunderstanding the avenues with which this gets installed. IOW, I'm surprised if its just some random thing that enough users would have it to be a top crasher. Maybe it's artificially high because of people downloading the latest firefox nightly for automation and running things in a sandbox for automation purposes, perhaps? Who knows. (I suppose I don't really know what kind of volume "firefox nightly top crasher" implies, though)

[glandium]

It's also #11 of Firefox beta crashes on Windows, FWIW.

[the8472]

Do those sandboxes intercept the calls as kernel drivers? Maybe microsoft could add some checks on their end to ensure that drivers uphold documented guarantees for userspace.

[thomcc]

Do those sandboxes intercept the calls as kernel drivers? Maybe microsoft could add some checks on their end to ensure that drivers uphold documented guarantees for userspace.

It's probably possible but tricky, I'm not an expert on windows kernel internals though, perhaps this isn't how it works.

Even if it is possible, I don't think they'd do this though -- it would break programs relying on it not being checked, sadly...

[badboy]

Looks like this is filed upstream already, though with less information, so cross-linking them here: sandboxie-plus/Sandboxie#2443