This is an archive of the discontinued LLVM Phabricator instance.

Win: handle \\?\UNC\ prefix in realPathFromHandle (PR43204)
ClosedPublic

Authored by hans on Sep 4 2019, 6:25 AM.

Details

Summary

After r361885, realPathFromHandle() ends up getting called on the working directory on each Clang invocation. This unveiled that the code didn't work for paths on network shares.

For example, if one maps the local dir c:\src\tmp to x:

net use x: \\localhost\c$\tmp

and run e.g. "clang -c foo.cc" in x:\, realPathFromHandle will get \\?\UNC\localhost\c$\src\tmp\ back from GetFinalPathNameByHandleW, and would strip off the initial \\?\ prefix, ending up with a path that doesn't work.

This patch makes the prefix stripping a little smarter to handle this case.

Diff Detail

Event Timeline

hans created this revision.Sep 4 2019, 6:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 4 2019, 6:25 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

Hi Hans, I am not too familiar with all windows path naming variants. Did you really observe a '\\?UNC\xxxx' path ?

llvm/lib/Support/Windows/Path.inc
379

For my testcase, I observe a:

\\?\UNC\server\path
   ^-- extra \

So, the check should be:

if (CountChars >= 8 && ::memcmp(Data, L"\\\\?\\UNC\\", 16) == 0) {

Later on, CountChars and Data should be reduced by '6'.

rnk added a comment.Sep 4 2019, 10:39 AM

As discussed, maybe we should try harder to revert whatever made us more dependent on this.

hans marked an inline comment as done.Sep 4 2019, 10:41 AM

Hi Hans, I am not too familiar with all windows path naming variants. Did you really observe a '\\?UNC\xxxx' path ?

You're right, I fat fingered it. Let me upload a new patch...

hans added a comment.Sep 4 2019, 10:44 AM
In D67166#1658031, @rnk wrote:

As discussed, maybe we should try harder to revert whatever made us more dependent on this.

I think we should do both. Even if we revert to become less dependent on this function, I think we should also try to fix the UNC case which is obviously broken here.

hans updated this revision to Diff 218748.Sep 4 2019, 10:50 AM
hans retitled this revision from Win: handle \\?UNC\ prefix in realPathFromHandle (PR43204) to Win: handle \\?\UNC\ prefix in realPathFromHandle (PR43204).
rnk accepted this revision.Sep 4 2019, 11:06 AM

lgtm

llvm/lib/Support/Windows/Path.inc
378

The mix of byte counts and character counts here is making me feel like this is too clever. You could do something like this:

template <size_t N>
static size_t wideStartsWith(const wchar_t *Str, const wchar_t (&Prefix)[N]) {
  return wcsncmp(Str, Prefix, N) == 0;
}

Alternatively it could take an ArrayRef<wchar_t> and use this memcmp pattern here.

This revision is now accepted and ready to land.Sep 4 2019, 11:06 AM
amccarth accepted this revision.Sep 4 2019, 1:21 PM

The \\?\ prefix tells the Windows API layer not to parse the path strings and just pass it along to the file system. Ramifications:

  1. You can no longer use . or .. as a path component. Those are treated as special in the API layer rather than as part of the file system.
  2. Many Windows APIs that take a path will accept / and treat it as \. If you use \\?\, the entire path must use \ throughout.
  3. The prefix also lets you use long path names (> MAX_PATH), but only if you use them with the W versions of the APIs (e.g., CreateFileW rather than CreateFileA). [On Windows 10, there are other ways to get long path names, but I don't think that's relevant here.]

I suspect the desire to ever strip the prefix stems from some of these ramifications.

The decision to include or strip this prefix can have all sorts of impact that we may not be able to anticipate.

But I don't see anything wrong with the code in this patch.

This revision was automatically updated to reflect the committed changes.