[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 ↗

(On Diff #247311)

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

84

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

llvm/unittests/Support/Path.cpp

1888

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

84

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.

135

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

675

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?

1881

const?

1904

This seems like a better place to define Expected.

[andrewng]

llvm/lib/Support/Windows/Path.inc

84

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.

135

Thanks for spotting this one!

llvm/unittests/Support/Path.cpp

675

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.

1881

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.

1888

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/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/include/llvm/Support/Windows/WindowsSupport.h

 }
 
 namespace windows {
 // Returns command line arguments. Unlike arguments given to main(),
 // this function guarantees that the returned arguments are encoded in
 // UTF-8 regardless of the current code page setting.
 std::error_code GetCommandLineArguments(SmallVectorImpl<const char *> &Args,
                                         BumpPtrAllocator &Alloc);
+
+/// 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,
+                          size_t MaxPathLen = MAX_PATH);
+
 } // end namespace windows
 } // 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::windows::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 windows {
 
-// 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,
+                          size_t MaxPathLen) {
+  assert(MaxPathLen <= MAX_PATH);
+
+  // 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;
+
+  const 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 *const LongPathPrefix = "\\\\?\\";
+
+  if ((Path16.size() + CurPathLen) < MaxPathLen ||
+      Path8Str.startswith(LongPathPrefix))
+    return std::error_code();
+
+  if (!IsAbsolute) {
+    if (EC = llvm::sys::fs::make_absolute(Path8Str))
+      return EC;
+
+  // Remove '.' and '..' because long paths treat these as real path components.
+  llvm::sys::path::remove_dots(Path8Str, true);
+
+  const 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.
+  const size_t RootNameSize = RootName.size();
+  if (RootNameSize < Path8Str.size() && Path8Str[RootNameSize] == '/')
+    Path8Str[RootNameSize] = '\\';
+
+  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);
+
 
-  // Just use the caller's original path.
-  return UTF8ToUTF16(Path8Str, Path16);
-}
-} // end namespace path
+} // end namespace windows
 
 namespace fs {
 
 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.
   if (Size == 0)
     return "";
 
   // Insufficient space is determined by a return value equal to the size of
   // the buffer passed in.
   if (Size == PathName.capacity())
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))
+  // CreateDirectoryW has a lower maximum path length as it must leave room for
+  // an 8.3 filename.
+  if (std::error_code ec = widenPath(path, path_utf16, MAX_PATH - 12))
     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::windows::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::windows::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

 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 #ifdef _WIN32
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Support/Chrono.h"
+#include "llvm/Support/Windows/WindowsSupport.h"
 #include <windows.h>
 #include <winerror.h>
 #endif
 
 #ifdef LLVM_ON_UNIX
 #include <pwd.h>
 #include <sys/stat.h>
 #endif
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) {
+  const std::wstring LongPathPrefix(L"\\\\?\\");
+
+  // Test that the length limit is checked against the UTF-16 length and not the
+  // UTF-8 length.
+  std::string Input("C:\\foldername\\");
+  const std::string Pi("\xcf\x80"); // UTF-8 lower case pi.
+  // Add Pi up to the MAX_PATH limit.
+  const 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);
+  SmallVector<wchar_t, MAX_PATH + 16> Result;
+  ASSERT_NO_ERROR(windows::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.
+  SmallVector<wchar_t, MAX_PATH + 16> Expected;
+  ASSERT_NO_ERROR(windows::UTF8ToUTF16(Input, Expected));
+  Expected.insert(Expected.begin(), LongPathPrefix.begin(),
+                  LongPathPrefix.end());
+
+  ASSERT_NO_ERROR(windows::widenPath(Input, Result));
+  EXPECT_EQ(Result, Expected);
+
+  // Test that UNC paths are handled correctly.
+  const std::string ShareName("\\\\sharename\\");
+  const std::string FileName("\\filename");
+  // Initialize directory name so that the input is within the MAX_PATH limit.
+  const char DirChar = 'x';
+  std::string DirName(MAX_PATH - ShareName.size() - FileName.size() - 1,
+                      DirChar);
+
+  Input = ShareName + DirName + FileName;
+  ASSERT_NO_ERROR(windows::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));
+  const std::wstring UNCPrefix(LongPathPrefix + L"UNC\\");
+  Expected.insert(Expected.begin(), UNCPrefix.begin(), UNCPrefix.end());
+
+  ASSERT_NO_ERROR(windows::widenPath(Input, Result));
+  EXPECT_EQ(Result, Expected);
+
+  // Check that Unix separators are handled correctly.
+  std::replace(Input.begin(), Input.end(), '\\', '/');
+  ASSERT_NO_ERROR(windows::widenPath(Input, Result));
+  EXPECT_EQ(Result, Expected);
+
+  // Check the removal of "dots".
+  Input = ShareName + DirName + "\\.\\foo\\.\\.." + FileName;
+  ASSERT_NO_ERROR(windows::widenPath(Input, Result));
+  EXPECT_EQ(Result, Expected);
+}
+#endif
+
 } // anonymous namespace