[Support] Fix Windows 7 path handling

[aganea]

Summary

As reported here: https://bugs.llvm.org/show_bug.cgi?id=48378#c0 and here: https://github.com/rust-lang/rust/issues/81051 since rG79657e2339b58bc01fe1b85a448bb073d57d90bb, some LLVM programs such as llvm-ar don't work properly on Windows 7.

The issue is shown in the snippet by Oleksandr Prodan: https://pastebin.com/v51m3uBU

In essence, once the 'Delete' flag has been set on a file, the file path can't be queried anymore with GetFinalPathNameByHandleW. This however works on Windows 10, GetFinalPathNameByHandleW returns sucessfully.

With this current patch, we simply prevent setting the flag on network files, but do not prevent from clearning it. All tests pass fine on Windows 7 & Windows 10. I've also tested @rdwampler 's case where I ran llvm-ar r empty.a something on a network mount. At the moment, we cannot specifically add a test coverage for this, since it requres mounting a network drive.

[rdwampler]

llvm/lib/Support/Windows/Path.inc

425

IIUC, realPathFromHandle() fails because FILE_DISPOSITION_INFO is already set to true, if so can you update the comment here to clarify. I think this would still fail if setDeleteDisposition() is called twice with Delete being true.

[amccarth]

I like the solution. Has anyone actually tried it on Windows 7? I ask because some of the observations noted in second hand descriptions of the problem didn't fully add up. I think we understand the root problem, but I'm not 100% certain.

[aganea]

As suggested by @rdwampler

[aganea]

In D99212#2645957, @amccarth wrote:

I like the solution. Has anyone actually tried it on Windows 7? I ask because some of the observations noted in second hand descriptions of the problem didn't fully add up. I think we understand the root problem, but I'm not 100% certain.

Yes, I've managed to repro the initial issue on a Windows 7 VM.
The problem is easily reproducible with this Python snippet: https://pastebin.com/v51m3uBU
Note the last two commands. GetFinalPathNameByHandleW fails on Windows 7 but not on Windows 10. If setting SetFileInformationByHandle with fdi.DeleteFile = False just before calling GetFinalPathNameByHandleW, things work as expected (even if we made several calls with fdi.DeleteFile = True before).

After patch, as suggedted by @rdwampler, I simply forced to always call Disposition.DeleteFile = false before doing anything else.

In the screenshot below, you can see what happens on Windows 7. The first three calls to SetDispositionInformationFile API are for a write to a local drive (corresponds to llvm-ar r empty.a d:\bug.py). The last two calls to SetDispositionInformationFile are for a write to a network drive (corresponds to the same command, but ran on Y: which is the mounted network drive)

Please take another look to the updated patch.

llvm/lib/Support/Windows/Path.inc

425

Changed as suggested. Please let me know if the wording isn't correct of it doesn't fully capture the issue.

[jhenderson]

Looks reasonable to me. I guess this means that on Windows 7 we might end up with temp files kicking around afterwards?

[aganea]

In D99212#2646852, @jhenderson wrote:

Looks reasonable to me. I guess this means that on Windows 7 we might end up with temp files kicking around afterwards?

There's no behavior specific to Windows 7, only this workaround to ensure the code works on both systems (Win 7 & 10). Temp files will remain on disk only if we're writing to a network drive, and only if the target file isn't renamed.

[jhenderson]

In D99212#2647555, @aganea wrote:

In D99212#2646852, @jhenderson wrote:

Looks reasonable to me. I guess this means that on Windows 7 we might end up with temp files kicking around afterwards?

There's no behavior specific to Windows 7, only this workaround to ensure the code works on both systems (Win 7 & 10). Temp files will remain on disk only if we're writing to a network drive, and only if the target file isn't renamed.

Yep, got it (I realised there wasn't anything Windows 7 specific here before that point). Not all temp files will be renamed though. Is it worth some additional commenting somewhere to make this clear?

[rdwampler]

llvm/lib/Support/Windows/Path.inc

425

Thanks, LGTM.

[aganea]

In D99212#2647586, @jhenderson wrote:

In D99212#2647555, @aganea wrote:

In D99212#2646852, @jhenderson wrote:

Looks reasonable to me. I guess this means that on Windows 7 we might end up with temp files kicking around afterwards?

There's no behavior specific to Windows 7, only this workaround to ensure the code works on both systems (Win 7 & 10). Temp files will remain on disk only if we're writing to a network drive, and only if the target file isn't renamed.

Yep, got it (I realised there wasn't anything Windows 7 specific here before that point). Not all temp files will be renamed though. Is it worth some additional commenting somewhere to make this clear?

Please see the updated diff.

[amccarth]

This looks even better.

I just found a comment in the Microsoft docs that suggests the problem _might_ have been avoided if any open handles to the file were created with FILE_SHARE_DELETE, but that sounds like a bigger change with a lot of potential side effects to think through, so I still prefer this localized fix.

In D99212#2646267, @aganea wrote:

Yes, I've managed to repro the initial issue on a Windows 7 VM.
The problem is easily reproducible with this Python snippet: https://pastebin.com/v51m3uBU

Yes, I understood the general issue. I was just trying to reconcile the details report on one of the bug threads (Rust?) that reported that it was the second SetFileInformationByHandle call that failed. That may have been true, but I suspect that was a cascading failure and the root problem was GetFInalPathNameByHandleW.

I don't have access to Windows 7, so I was unable to try it myself.

[aganea]

Commited as rG64ab2b6825c5 (doh, I forgot the Differential Revision: part).

llvm/lib/Support/Windows/Path.inc

std::error_code is_local(int FD, bool &Result) {
 
   if (std::error_code EC = realPathFromHandle(Handle, FinalPath))
     return EC;
 
   return is_local_internal(FinalPath, Result);
 }
 
 static std::error_code setDeleteDisposition(HANDLE Handle, bool Delete) {
-  // First, check if the file is on a network (non-local) drive. If so, don't
-  // set DeleteFile to true, since it prevents opening the file for writes.
+  // Clear the FILE_DISPOSITION_INFO flag first, before checking if it's a
+  // network file. On Windows 7 the function realPathFromHandle() below fails
+  // if the FILE_DISPOSITION_INFO flag was already set by a prior call.
+  FILE_DISPOSITION_INFO Disposition;
+  Disposition.DeleteFile = false;
+  if (!SetFileInformationByHandle(Handle, FileDispositionInfo, &Disposition,
+                                  sizeof(Disposition)))
+    return mapWindowsError(::GetLastError());
+  if (!Delete)
+    return std::error_code();
+
+  // Check if the file is on a network (non-local) drive. If so, don't
+  // continue when DeleteFile is true, since it prevents opening the file for
+  // writes. Note -- this will leak temporary files on disk, but only when the
+  // target file is on a network drive.
   SmallVector<wchar_t, 128> FinalPath;
   if (std::error_code EC = realPathFromHandle(Handle, FinalPath))
     return EC;
 
   bool IsLocal;
   if (std::error_code EC = is_local_internal(FinalPath, IsLocal))
     return EC;
 
   if (!IsLocal)
     return std::error_code();
 
-  // The file is on a local drive, set the DeleteFile to true.
-  FILE_DISPOSITION_INFO Disposition;
-  Disposition.DeleteFile = Delete;
+  // The file is on a local drive, we can safely set FILE_DISPOSITION_INFO's
+  // flag.
+  Disposition.DeleteFile = true;
   if (!SetFileInformationByHandle(Handle, FileDispositionInfo, &Disposition,
                                   sizeof(Disposition)))
     return mapWindowsError(::GetLastError());
   return std::error_code();
 }
 
 static std::error_code rename_internal(HANDLE FromHandle, const Twine &To,
                                        bool ReplaceIfExists) {