[Support] Improve Windows widenPath and add support for long UNC paths

[andrewng]

Summary

Check the path length limit against the length of the UTF-16 version of
the input rather than the UTF-8 equivalent, as the UTF-16 length may be
shorter. Move widenPath from the llvm::sys::path namespace in Path.h to
the llvm::sys::windows namespace in WindowsSupport.h. Only use the
reduced path length limit for create directory. Canonicalize using
sys::path::remove_dots().

[rnk]

llvm/include/llvm/Support/FileSystem.h

1161

Let's move this somewhere more obscure so clients like llvm-ar are less likely to find it and try to use it. I'd suggest WindowsSupport.h. Ideally we would make it local to Support, but we don't have any existing internal support headers.

llvm/lib/Support/Windows/Path.inc

82

Why not always use the lower limit? What is the harm in using the UNC prefixes slightly more often?

llvm/unittests/Support/Path.cpp

1887

Is this the first introduction of a real UTF-8 source character? Are you confident that this will work portably? Do you need to write u8"pi" to make this work where utf-8 is not the local codepage?

[amccarth]

I'm not sure I agree with moving widen from sys::path to sys::fs. Is there consensus on the layering here? This adds dependencies for sys::fs on sys::path, but I would expect the dependencies to be in the other direction--the path style you use would depend on the underlying file system, not the other way around, right? The long path prefix on an absolute local path depends on the underlying file system being NTFS.

And, in my recent experience sys::path::is_absolute(..., sys::path::style::windows) seems buggy with UNC paths, so I'm skeptical if it works right with the long path prefix.

llvm/lib/Support/Windows/Path.inc

82

The long path prefix isn't a UNC prefix. The long path prefix is an escape that tells many system APIs not to try to interpret the string as a file system path, but simply to pass it on to the underlying file system as an opaque string. That's great if you underlying file system is NTFS, that's great. I don't know whether something like a FAT32 flash drive handles that. If not, then there could be some harm to prematurely appending the long path prefix. That's probably pretty low risk.

I think the ForCreateDir parameter makes this API a leaky abstraction and the risk is low. If you insist on having a way to enforce different limits, then I'd make the parameter be the path limit you want, default it to MAX_PATH, and let the special cases (like code that calls CreateDirectoryW) explain why they're setting it lower.

117

Would it be appropriate to check (or at least assert) that RootName.size() <= Path8Str.size()? If somehow the entire input string could be considered the root name, then this reference would be out of bounds.

llvm/unittests/Support/Path.cpp

674

Does this patch, which allows longer paths on Windows, invalidate this test which checks that PATH_MAX is enforced? Presumably, createUniqueFile could use the new widen. Or is the issue here that PATH_MAX also applied to each component of a path?

1880

const?

1903

This seems like a better place to define Expected.

[andrewng]

llvm/lib/Support/Windows/Path.inc

82

I think anything to help avoid having to mangle the input is beneficial. The need for this type of code is already bad enough.

The real purpose of this function is to best prepare the path for the Win32 file APIs. It's not meant to be any kind of validation. So the Win32 file APIs could still fail for various reasons, such as other file system constraints.

I like the idea to change ForCreateDir to MaxPathLen, thanks.

117

Thanks for spotting this one!

llvm/unittests/Support/Path.cpp

674

As you've said, I believe that there is also a length limit to each component of a path, although I'm not sure what that exact limit is.

1880

My usual style is to liberally use const where appropriate. But I've had objections to that in the past. As you've mentioned it here, I'll go back to my usual style and see if anyone objects.

1887

Good point. It should be OK for all supported Win32 compilers, but to be on the safe side, I'll change it to hex representation to be sure.

[andrewng]

Updated to address review comments and suggestions.

[andrewng]

Ping.

[andrewng]

Ping.

[rnk]

LGTM

I like Adrian's suggestion of having the caller pass in the maximum path length, thanks!

llvm/include/llvm/Support/FileSystem.h

 /// Note: Windows reports results according to the quota allocated to the user.
 ///
 /// @param Path Input path.
 /// @returns a space_info structure filled with the capacity, free, and
 /// available space on the device \a Path is on. A platform specific error_code
 /// is returned on error.
 ErrorOr<space_info> disk_space(const Twine &Path);
 
+#ifdef _WIN32
+/// Convert UTF-8 path to a suitable UTF-16 path for use with the Win32 Unicode
+/// File API.
+std::error_code widenPath(const Twine &Path8, SmallVectorImpl<wchar_t> &Path16,
+                          bool ForCreateDir = false);
+#endif
+
 /// This class represents a memory mapped file. It is based on
 /// boost::iostreams::mapped_file.
 class mapped_file_region {
 public:
   enum mapmode {
     readonly, ///< May only access map via const_data as read only.
     readwrite, ///< May access map via data and modify it. Written to path.
     priv ///< May modify via data, but changes are lost on destruction.

llvm/include/llvm/Support/Path.h

 ///
 /// @param path processed path
 /// @param remove_dot_dot specify if '../' (except for leading "../") should be
 /// removed
 /// @result True if path was changed
 bool remove_dots(SmallVectorImpl<char> &path, bool remove_dot_dot = false,
                  Style style = Style::native);
 
-#if defined(_WIN32)
-std::error_code widenPath(const Twine &Path8, SmallVectorImpl<wchar_t> &Path16);
-#endif
-
 } // end namespace path
 } // end namespace sys
 } // end namespace llvm
 
 #endif

llvm/lib/Support/Windows/Path.inc

 # pragma comment(lib, "ole32.lib")     // This provides CoTaskMemFree
 #endif
 
 using namespace llvm;
 
 using llvm::sys::windows::UTF8ToUTF16;
 using llvm::sys::windows::CurCPToUTF16;
 using llvm::sys::windows::UTF16ToUTF8;
-using llvm::sys::path::widenPath;
+using llvm::sys::fs::widenPath;
 
 static bool is_separator(const wchar_t value) {
   switch (value) {
   case L'\\':
   case L'/':
     return true;
   default:
     return false;
   }
 }
 
 namespace llvm {
 namespace sys  {
-namespace path {
+namespace fs {
 
-// Convert a UTF-8 path to UTF-16.  Also, if the absolute equivalent of the
-// path is longer than CreateDirectory can tolerate, make it absolute and
-// prefixed by '\\?\'.
-std::error_code widenPath(const Twine &Path8,
-                          SmallVectorImpl<wchar_t> &Path16) {
-  const size_t MaxDirLen = MAX_PATH - 12; // Must leave room for 8.3 filename.
-
-  // Several operations would convert Path8 to SmallString; more efficient to
-  // do it once up front.
-  SmallString<128> Path8Str;
+// Convert a UTF-8 path to UTF-16. Also, if the absolute equivalent of the path
+// is longer than the limit that the Win32 Unicode File API can tolerate, make
+// it an absolute normalized path prefixed by '\\?\'.
+std::error_code widenPath(const Twine &Path8, SmallVectorImpl<wchar_t> &Path16,
+                          bool ForCreateDir) {
+  // Several operations would convert Path8 to SmallString; more efficient to do
+  // it once up front.
+  SmallString<MAX_PATH> Path8Str;
   Path8.toVector(Path8Str);
 
-  // If we made this path absolute, how much longer would it get?
+  std::error_code EC = UTF8ToUTF16(Path8Str, Path16);
+  if (EC)
+    return EC;
+
+  // The limit for the absolute path including the null terminator.
+  size_t MaxDirLen =
+      ForCreateDir        // Win32 CreateDirectory API has a lower limit.
+          ? MAX_PATH - 12 // Must leave room for 8.3 filename.
+          : MAX_PATH;
+
+  bool IsAbsolute = llvm::sys::path::is_absolute(Path8);
   size_t CurPathLen;
-  if (llvm::sys::path::is_absolute(Twine(Path8Str)))
+  if (IsAbsolute)
     CurPathLen = 0; // No contribution from current_path needed.
   else {
-    CurPathLen = ::GetCurrentDirectoryW(0, NULL);
+    CurPathLen = ::GetCurrentDirectoryW(
+        0, NULL); // Returns the size including the null terminator.
     if (CurPathLen == 0)
       return mapWindowsError(::GetLastError());
   }
 
-  // Would the absolute path be longer than our limit?
-  if ((Path8Str.size() + CurPathLen) >= MaxDirLen &&
-      !Path8Str.startswith("\\\\?\\")) {
-    SmallString<2*MAX_PATH> FullPath("\\\\?\\");
-    if (CurPathLen) {
-      SmallString<80> CurPath;
-      if (std::error_code EC = llvm::sys::fs::current_path(CurPath))
-        return EC;
-      FullPath.append(CurPath);
-    }
-    // Traverse the requested path, canonicalizing . and .. (because the \\?\
-    // prefix is documented to treat them as real components).  Ignore
-    // separators, which can be returned from the iterator if the path has a
-    // drive name.  We don't need to call native() on the result since append()
-    // always attaches preferred_separator.
-    for (llvm::sys::path::const_iterator I = llvm::sys::path::begin(Path8Str),
-                                         E = llvm::sys::path::end(Path8Str);
-                                         I != E; ++I) {
-      if (I->size() == 1 && is_separator((*I)[0]))
-        continue;
-      if (I->size() == 1 && *I == ".")
-        continue;
-      if (I->size() == 2 && *I == "..")
-        llvm::sys::path::remove_filename(FullPath);
-      else
-        llvm::sys::path::append(FullPath, *I);
-    }
-    return UTF8ToUTF16(FullPath, Path16);
-  }
+  const char *LongPathPrefix = "\\\\?\\";
 
-  // Just use the caller's original path.
-  return UTF8ToUTF16(Path8Str, Path16);
+  if ((Path16.size() + CurPathLen) < MaxDirLen ||
+      Path8Str.startswith(LongPathPrefix))
+    return std::error_code();
+
+  if (!IsAbsolute) {
+    if (EC = llvm::sys::fs::make_absolute(Path8Str))
+      return EC;
   }
-} // end namespace path
 
-namespace fs {
+  // Remove '.' and '..' because long paths treat these as real path components.
+  llvm::sys::path::remove_dots(Path8Str, true);
+
+  StringRef RootName = llvm::sys::path::root_name(Path8Str);
+  assert(!RootName.empty() &&
+         "Root name cannot be empty for an absolute path!");
+
+  // llvm::sys::path::remove_dots, used above, can leave a '/' after the root
+  // name and long paths must use '\' as the separator.
+  if (Path8Str[RootName.size()] == '/')
+    Path8Str[RootName.size()] = '\\';
+
+  SmallString<2 * MAX_PATH> FullPath(LongPathPrefix);
+  if (RootName[1] != ':') { // Check if UNC.
+    FullPath.append("UNC\\");
+    FullPath.append(Path8Str.begin() + 2, Path8Str.end());
+  } else
+    FullPath.append(Path8Str);
+
+  return UTF8ToUTF16(FullPath, Path16);
+}
 
 const file_t kInvalidFile = INVALID_HANDLE_VALUE;
 
 std::string getMainExecutable(const char *argv0, void *MainExecAddr) {
   SmallVector<wchar_t, MAX_PATH> PathName;
   DWORD Size = ::GetModuleFileNameW(NULL, PathName.data(), PathName.capacity());
 
   // A zero return value indicates a failure other than insufficient space.
std::error_code set_current_path(const Twine &path) {
 
   return std::error_code();
 }
 
 std::error_code create_directory(const Twine &path, bool IgnoreExisting,
                                  perms Perms) {
   SmallVector<wchar_t, 128> path_utf16;
 
-  if (std::error_code ec = widenPath(path, path_utf16))
+  if (std::error_code ec = widenPath(path, path_utf16, true))
     return ec;
 
   if (!::CreateDirectoryW(path_utf16.begin(), NULL)) {
     DWORD LastError = ::GetLastError();
     if (LastError != ERROR_ALREADY_EXISTS || !IgnoreExisting)
       return mapWindowsError(LastError);
   }
 

llvm/lib/Support/Windows/Program.inc

static HANDLE RedirectIO(Optional<StringRef> Path, int fd,
   sa.bInheritHandle = TRUE;
 
   SmallVector<wchar_t, 128> fnameUnicode;
   if (Path->empty()) {
     // Don't play long-path tricks on "NUL".
     if (windows::UTF8ToUTF16(fname, fnameUnicode))
       return INVALID_HANDLE_VALUE;
   } else {
-    if (path::widenPath(fname, fnameUnicode))
+    if (sys::fs::widenPath(fname, fnameUnicode))
       return INVALID_HANDLE_VALUE;
   }
   h = CreateFileW(fnameUnicode.data(), fd ? GENERIC_WRITE : GENERIC_READ,
                   FILE_SHARE_READ, &sa, fd == 0 ? OPEN_EXISTING : CREATE_ALWAYS,
                   FILE_ATTRIBUTE_NORMAL, NULL);
   if (h == INVALID_HANDLE_VALUE) {
     MakeErrMsg(ErrMsg, fname + ": Can't open file for " +
         (fd ? "input" : "output"));
static bool Execute(ProcessInfo &PI, StringRef Program,
 
   PROCESS_INFORMATION pi;
   memset(&pi, 0, sizeof(pi));
 
   fflush(stdout);
   fflush(stderr);
 
   SmallVector<wchar_t, MAX_PATH> ProgramUtf16;
-  if (std::error_code ec = path::widenPath(Program, ProgramUtf16)) {
+  if (std::error_code ec = sys::fs::widenPath(Program, ProgramUtf16)) {
     SetLastError(ec.value());
     MakeErrMsg(ErrMsg,
                std::string("Unable to convert application name to UTF-16"));
     return false;
   }
 
   SmallVector<wchar_t, MAX_PATH> CommandUtf16;
   if (std::error_code ec = windows::UTF8ToUTF16(Command, CommandUtf16)) {

llvm/unittests/Support/Path.cpp

TEST_F(FileSystemTest, TempFiles) {
   // Remove the hard link.
   ASSERT_NO_ERROR(fs::remove(Twine(TempPath2)));
 
   // Make sure Temp1 doesn't exist.
   ASSERT_EQ(fs::access(Twine(TempPath), sys::fs::AccessMode::Exist),
             errc::no_such_file_or_directory);
 
 #ifdef _WIN32
   // Path name > 260 chars should get an error.
   const char *Path270 =
     "abcdefghijklmnopqrstuvwxyz9abcdefghijklmnopqrstuvwxyz8"
     "abcdefghijklmnopqrstuvwxyz7abcdefghijklmnopqrstuvwxyz6"
     "abcdefghijklmnopqrstuvwxyz5abcdefghijklmnopqrstuvwxyz4"
     "abcdefghijklmnopqrstuvwxyz3abcdefghijklmnopqrstuvwxyz2"
     "abcdefghijklmnopqrstuvwxyz1abcdefghijklmnopqrstuvwxyz0";
   EXPECT_EQ(fs::createUniqueFile(Path270, FileDescriptor, TempPath),
             errc::invalid_argument);

 #endif // !FreeBSD && !NetBSD && !OpenBSD && !AIX
 
   EXPECT_EQ(fs::setPermissions(TempPath, fs::all_perms & ~fs::sticky_bit),
                                NoError);
   EXPECT_TRUE(CheckPermissions(fs::all_perms & ~fs::sticky_bit));
 #endif
 }
 
+#ifdef _WIN32
+TEST_F(FileSystemTest, widenPath) {
+  std::wstring LongPathPrefix(L"\\\\?\\");
+  SmallVector<wchar_t, MAX_PATH + 16> Result;
+  SmallVector<wchar_t, MAX_PATH + 16> Expected;
+
+  // Test that the length limit is checked against the UTF-16 length and not the
+  // UTF-8 length.
+  std::string Input("C:\\foldername\\");
+  std::string Pi("π"); // UTF-8: 0xCF 0x80
+  // Add Pi up to the MAX_PATH limit.
+  size_t NumChars = MAX_PATH - Input.size() - 1;
+  for (size_t i = 0; i < NumChars; ++i)
+    Input += Pi;
+  // Check that UTF-8 length already exceeds MAX_PATH.
+  EXPECT_TRUE(Input.size() > MAX_PATH);
+  ASSERT_NO_ERROR(fs::widenPath(Input, Result));
+  // Result should not start with the long path prefix.
+  EXPECT_TRUE(std::wmemcmp(Result.data(), LongPathPrefix.c_str(),
+                           LongPathPrefix.size()) != 0);
+  EXPECT_EQ(Result.size(), MAX_PATH - 1);
+
+  // Add another Pi to exceed the MAX_PATH limit.
+  Input += Pi;
+  // Construct the expected result.
+  ASSERT_NO_ERROR(windows::UTF8ToUTF16(Input, Expected));
+  Expected.insert(Expected.begin(), LongPathPrefix.begin(),
+                  LongPathPrefix.end());
+
+  ASSERT_NO_ERROR(fs::widenPath(Input, Result));
+  EXPECT_EQ(Result, Expected);
+
+  // Test that UNC paths are handled correctly.
+  std::string ShareName("\\\\sharename\\");
+  std::string FileName("\\filename");
+  // Initialize directory name so that the input is within the MAX_PATH limit.
+  char DirChar = 'x';
+  std::string DirName(MAX_PATH - ShareName.size() - FileName.size() - 1,
+                      DirChar);
+
+  Input = ShareName + DirName + FileName;
+  ASSERT_NO_ERROR(fs::widenPath(Input, Result));
+  // Result should not start with the long path prefix.
+  EXPECT_TRUE(std::wmemcmp(Result.data(), LongPathPrefix.c_str(),
+                           LongPathPrefix.size()) != 0);
+  EXPECT_EQ(Result.size(), MAX_PATH - 1);
+
+  // Extend the directory name so the input exceeds the MAX_PATH limit.
+  DirName += DirChar;
+  Input = ShareName + DirName + FileName;
+  // Construct the expected result.
+  ASSERT_NO_ERROR(windows::UTF8ToUTF16(StringRef(Input).substr(2), Expected));
+  std::wstring UNCPrefix(LongPathPrefix + L"UNC\\");
+  Expected.insert(Expected.begin(), UNCPrefix.begin(), UNCPrefix.end());
+
+  ASSERT_NO_ERROR(fs::widenPath(Input, Result));
+  EXPECT_EQ(Result, Expected);
+
+  // Check that Unix separators are handled correctly.
+  std::replace(Input.begin(), Input.end(), '\\', '/');
+  ASSERT_NO_ERROR(fs::widenPath(Input, Result));
+  EXPECT_EQ(Result, Expected);
+
+  // Check the removal of "dots".
+  Input = ShareName + DirName + "\\.\\foo\\.\\.." + FileName;
+  ASSERT_NO_ERROR(fs::widenPath(Input, Result));
+  EXPECT_EQ(Result, Expected);
+}
+#endif
+
 } // anonymous namespace