This is an archive of the discontinued LLVM Phabricator instance.

[Support] Fix Windows 7 path handling
ClosedPublic

Authored by aganea on Mar 23 2021, 1:22 PM.

Details

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.

Diff Detail

Event Timeline

aganea created this revision.Mar 23 2021, 1:22 PM
aganea requested review of this revision.Mar 23 2021, 1:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2021, 1:22 PM
aganea edited the summary of this revision. (Show Details)Mar 23 2021, 1:30 PM
rdwampler added inline comments.Mar 23 2021, 1:35 PM
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 accepted this revision.Mar 23 2021, 1:54 PM

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.

This revision is now accepted and ready to land.Mar 23 2021, 1:54 PM
aganea updated this revision to Diff 332815.Mar 23 2021, 4:02 PM
aganea marked an inline comment as done.

As suggested by @rdwampler

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.

aganea requested review of this revision.Mar 23 2021, 4:19 PM
jhenderson accepted this revision.Mar 24 2021, 1:00 AM

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

This revision is now accepted and ready to land.Mar 24 2021, 1:00 AM

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.

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 added inline comments.Mar 24 2021, 7:04 AM
llvm/lib/Support/Windows/Path.inc
425

Thanks, LGTM.

aganea updated this revision to Diff 332972.Mar 24 2021, 7:05 AM

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 accepted this revision.Mar 24 2021, 9:11 AM

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.

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 closed this revision.Mar 24 2021, 9:57 AM

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