Windows' `symlink_junction_inner` mysteriously fails

[ChrisDenton]

I think symlink_junction_inner sometimes isn't writing to the buffer properly which can cause the DeviceIoControl to fail and thus tests that use it. This function isn't great and is only used in tests. I can reproduce this consistently locally on current master but in CI most PRs seem to test ok (though not all).

I've not really investigated yet but on a hunch I added the following which seems to fix it locally, I don't know why or if it's a fluke:

diff --git a/library/std/src/sys/windows/fs.rs b/library/std/src/sys/windows/fs.rs
index 37809803828..d7f1a44bdf4 100644
--- a/library/std/src/sys/windows/fs.rs
+++ b/library/std/src/sys/windows/fs.rs
@@ -1409,6 +1409,8 @@ fn symlink_junction_inner(original: &Path, junction: &Path) -> io::Result<()> {
         (*db).ReparseTargetLength = ((i - 1) * 2) as c::WORD;
         (*db).ReparseDataLength = (*db).ReparseTargetLength as c::DWORD + 12;

+        crate::hint::black_box(slice::from_raw_parts(buf, i));
+
         let mut ret = 0;
         cvt(c::DeviceIoControl(
             h as *mut _,

[workingjubilee]

cursed.

[ChrisDenton]

Huh, simplifying that change to crate::hint::black_box(buf); seems to work. So I guess it just needs something to touch that pointer that it can't optimize.

[ChrisDenton]

I had another quick thought and replaced the for loop with the for_each iterator function. That seems to fix it too. Some bad interaction between iterators and loops perhaps?

diff --git a/library/std/src/sys/windows/fs.rs b/library/std/src/sys/windows/fs.rs
index 37809803828..f68cf40c065 100644
--- a/library/std/src/sys/windows/fs.rs
+++ b/library/std/src/sys/windows/fs.rs
@@ -1398,10 +1398,10 @@ fn symlink_junction_inner(original: &Path, junction: &Path) -> io::Result<()> {
         // FIXME: this conversion is very hacky
         let v = br"\??\";
         let v = v.iter().map(|x| *x as u16);
-        for c in v.chain(original.as_os_str().encode_wide()) {
+        v.chain(original.as_os_str().encode_wide()).for_each(|c| {
             *buf.add(i) = c;
             i += 1;
-        }
+        });
         *buf.add(i) = 0;
         i += 1;
         (*db).ReparseTag = c::IO_REPARSE_TAG_MOUNT_POINT;

[clubby789]

The IOCTL docs don't state the type that should be passed, but links to REPARSE_DATA_BUFFER. This structure (with the MountPointReparseBuffer union member) is somewhat similar to our struct, but is definitely different. So it might be that we're just passing a wrongly laid out structure which would explain differing behaviour on different machines.

EDIT: Looks similar to this Wine patch.

[ChrisDenton]

Hm, this has been the same since forever in Rust so it'd be surprising for it to suddenly break now.

Oh! But now I've woken up I realised something. We're not zeroing the structure. I can't believe I missed that! So we're likely putting random data into the reserved parameters.

[ChrisDenton]

But yes, this function is awful and should be rewritten. Good job it's only used in tests...